* [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
@ 2021-09-08 6:59 ` Richard Zhu
0 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 6:59 UTC (permalink / raw)
To: l.stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel,
Richard Zhu
No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 82 +++++++++++++++++----------
1 file changed, 51 insertions(+), 31 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 80fc98acf097..0264432e4c4a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
+
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
{
struct dw_pcie *pci = imx6_pcie->pci;
@@ -498,33 +500,12 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
}
}
- ret = clk_prepare_enable(imx6_pcie->pcie_phy);
- if (ret) {
- dev_err(dev, "unable to enable pcie_phy clock\n");
- goto err_pcie_phy;
- }
-
- ret = clk_prepare_enable(imx6_pcie->pcie_bus);
+ ret = imx6_pcie_clk_enable(imx6_pcie);
if (ret) {
- dev_err(dev, "unable to enable pcie_bus clock\n");
- goto err_pcie_bus;
+ dev_err(dev, "unable to enable pcie clocks\n");
+ goto err_clks;
}
- ret = clk_prepare_enable(imx6_pcie->pcie);
- if (ret) {
- dev_err(dev, "unable to enable pcie clock\n");
- goto err_pcie;
- }
-
- ret = imx6_pcie_enable_ref_clk(imx6_pcie);
- if (ret) {
- dev_err(dev, "unable to enable pcie ref clock\n");
- goto err_ref_clk;
- }
-
- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value_cansleep(imx6_pcie->reset_gpio,
@@ -578,13 +559,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)
@@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
usleep_range(1000, 10000);
}
+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;
+
+ ret = clk_prepare_enable(imx6_pcie->pcie_phy);
+ if (ret) {
+ dev_err(dev, "unable to enable pcie_phy clock\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(imx6_pcie->pcie_bus);
+ if (ret) {
+ dev_err(dev, "unable to enable pcie_bus clock\n");
+ goto err_pcie_bus;
+ }
+
+ ret = clk_prepare_enable(imx6_pcie->pcie);
+ if (ret) {
+ dev_err(dev, "unable to enable pcie clock\n");
+ goto err_pcie;
+ }
+
+ ret = imx6_pcie_enable_ref_clk(imx6_pcie);
+ if (ret) {
+ dev_err(dev, "unable to enable pcie ref clock\n");
+ goto err_ref_clk;
+ }
+
+ /* 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 imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
{
clk_disable_unprepare(imx6_pcie->pcie);
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump
2021-09-08 6:59 ` Richard Zhu
@ 2021-09-08 6:59 ` Richard Zhu
-1 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 6:59 UTC (permalink / raw)
To: l.stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel,
Richard Zhu
Since there is error return check of the host_init callback, add error
check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
function.
Because that i.MX PCIe doesn't support the hot-plug feature. To save
power consumption as much as possible, turn off the clocks and power
supplies when the PCIe PHY link is never came up in probe procedure.
When PCIe link is never came up and vpcie regulator is present, there
would be following dump when try to put the regulator.
Disable this regulator to fix this dump when link is never came up.
[ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
[ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
[ 2.348160] ------------[ cut here ]------------
[ 2.352778] WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
[ 2.362184] Modules linked in:
[ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
[ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT)
[ 2.380040] Workqueue: events_unbound async_run_entry_fn
[ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[ 2.391369] pc : _regulator_put.part.0+0x14c/0x158
[ 2.396163] lr : regulator_put+0x34/0x48
[ 2.400088] sp : ffff8000122ebb30
[ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
[ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
[ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
[ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
[ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108
[ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
[ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
[ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
[ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
[ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
[ 2.474847] Call trace:
[ 2.477295] _regulator_put.part.0+0x14c/0x158
[ 2.481742] regulator_put+0x34/0x48
[ 2.485322] devm_regulator_release+0x10/0x18
[ 2.489681] release_nodes+0x38/0x60
[ 2.493262] devres_release_all+0x88/0xd0
[ 2.497276] really_probe+0xd0/0x2e8
[ 2.500858] __driver_probe_device+0x74/0xd8
[ 2.505137] driver_probe_device+0x7c/0x108
[ 2.509325] __device_attach_driver+0x8c/0xd0
[ 2.513685] bus_for_each_drv+0x74/0xc0
[ 2.517531] __device_attach_async_helper+0xb4/0xd8
[ 2.522419] async_run_entry_fn+0x30/0x100
[ 2.526521] process_one_work+0x19c/0x320
[ 2.530532] worker_thread+0x48/0x418
[ 2.534199] kthread+0x14c/0x158
[ 2.537432] ret_from_fork+0x10/0x18
[ 2.541013] ---[ end trace 3664ca4a50ce849b ]---
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 28 +++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0264432e4c4a..129928e42f84 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -144,6 +144,7 @@ struct imx6_pcie {
#define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
+static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
{
@@ -485,24 +486,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;
}
@@ -557,7 +558,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) {
@@ -566,6 +567,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)
@@ -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
imx6_pcie_reset_phy(imx6_pcie);
+ imx6_pcie_clk_disable(imx6_pcie);
+ if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+ regulator_disable(imx6_pcie->vpcie);
return ret;
}
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump
@ 2021-09-08 6:59 ` Richard Zhu
0 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 6:59 UTC (permalink / raw)
To: l.stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel,
Richard Zhu
Since there is error return check of the host_init callback, add error
check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
function.
Because that i.MX PCIe doesn't support the hot-plug feature. To save
power consumption as much as possible, turn off the clocks and power
supplies when the PCIe PHY link is never came up in probe procedure.
When PCIe link is never came up and vpcie regulator is present, there
would be following dump when try to put the regulator.
Disable this regulator to fix this dump when link is never came up.
[ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
[ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
[ 2.348160] ------------[ cut here ]------------
[ 2.352778] WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
[ 2.362184] Modules linked in:
[ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
[ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT)
[ 2.380040] Workqueue: events_unbound async_run_entry_fn
[ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[ 2.391369] pc : _regulator_put.part.0+0x14c/0x158
[ 2.396163] lr : regulator_put+0x34/0x48
[ 2.400088] sp : ffff8000122ebb30
[ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
[ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
[ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
[ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
[ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108
[ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
[ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
[ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
[ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
[ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
[ 2.474847] Call trace:
[ 2.477295] _regulator_put.part.0+0x14c/0x158
[ 2.481742] regulator_put+0x34/0x48
[ 2.485322] devm_regulator_release+0x10/0x18
[ 2.489681] release_nodes+0x38/0x60
[ 2.493262] devres_release_all+0x88/0xd0
[ 2.497276] really_probe+0xd0/0x2e8
[ 2.500858] __driver_probe_device+0x74/0xd8
[ 2.505137] driver_probe_device+0x7c/0x108
[ 2.509325] __device_attach_driver+0x8c/0xd0
[ 2.513685] bus_for_each_drv+0x74/0xc0
[ 2.517531] __device_attach_async_helper+0xb4/0xd8
[ 2.522419] async_run_entry_fn+0x30/0x100
[ 2.526521] process_one_work+0x19c/0x320
[ 2.530532] worker_thread+0x48/0x418
[ 2.534199] kthread+0x14c/0x158
[ 2.537432] ret_from_fork+0x10/0x18
[ 2.541013] ---[ end trace 3664ca4a50ce849b ]---
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 28 +++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 0264432e4c4a..129928e42f84 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -144,6 +144,7 @@ struct imx6_pcie {
#define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
+static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
{
@@ -485,24 +486,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;
}
@@ -557,7 +558,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) {
@@ -566,6 +567,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)
@@ -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
imx6_pcie_reset_phy(imx6_pcie);
+ imx6_pcie_clk_disable(imx6_pcie);
+ if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+ regulator_disable(imx6_pcie->vpcie);
return ret;
}
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump
2021-09-08 6:59 ` Richard Zhu
@ 2021-09-08 8:42 ` Lucas Stach
-1 siblings, 0 replies; 25+ messages in thread
From: Lucas Stach @ 2021-09-08 8:42 UTC (permalink / raw)
To: Richard Zhu, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> Since there is error return check of the host_init callback, add error
> check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> function.
>
> Because that i.MX PCIe doesn't support the hot-plug feature. To save
> power consumption as much as possible, turn off the clocks and power
> supplies when the PCIe PHY link is never came up in probe procedure.
>
> When PCIe link is never came up and vpcie regulator is present, there
> would be following dump when try to put the regulator.
> Disable this regulator to fix this dump when link is never came up.
>
> [ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
> [ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
> [ 2.348160] ------------[ cut here ]------------
> [ 2.352778] WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
> [ 2.362184] Modules linked in:
> [ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> [ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT)
> [ 2.380040] Workqueue: events_unbound async_run_entry_fn
> [ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> [ 2.391369] pc : _regulator_put.part.0+0x14c/0x158
> [ 2.396163] lr : regulator_put+0x34/0x48
> [ 2.400088] sp : ffff8000122ebb30
> [ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
> [ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
> [ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
> [ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
> [ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108
> [ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
> [ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
> [ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
> [ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
> [ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
> [ 2.474847] Call trace:
> [ 2.477295] _regulator_put.part.0+0x14c/0x158
> [ 2.481742] regulator_put+0x34/0x48
> [ 2.485322] devm_regulator_release+0x10/0x18
> [ 2.489681] release_nodes+0x38/0x60
> [ 2.493262] devres_release_all+0x88/0xd0
> [ 2.497276] really_probe+0xd0/0x2e8
> [ 2.500858] __driver_probe_device+0x74/0xd8
> [ 2.505137] driver_probe_device+0x7c/0x108
> [ 2.509325] __device_attach_driver+0x8c/0xd0
> [ 2.513685] bus_for_each_drv+0x74/0xc0
> [ 2.517531] __device_attach_async_helper+0xb4/0xd8
> [ 2.522419] async_run_entry_fn+0x30/0x100
> [ 2.526521] process_one_work+0x19c/0x320
> [ 2.530532] worker_thread+0x48/0x418
> [ 2.534199] kthread+0x14c/0x158
> [ 2.537432] ret_from_fork+0x10/0x18
> [ 2.541013] ---[ end trace 3664ca4a50ce849b ]---
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 28 +++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0264432e4c4a..129928e42f84 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -144,6 +144,7 @@ struct imx6_pcie {
> #define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
>
> static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
>
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> {
> @@ -485,24 +486,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;
Why do we need a separate variable for the error code here? Why not
just use the existing 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;
> + 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;
> }
>
> @@ -557,7 +558,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) {
> @@ -566,6 +567,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)
> @@ -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> imx6_pcie_reset_phy(imx6_pcie);
> + imx6_pcie_clk_disable(imx6_pcie);
This is a separate fix for the clock enable counts, that isn't
mentioned in the commit message.
It seems like this patch fixes in fact 3 issues: error propagation from
host_init, fix regulator reference handling, fix clk reference
handling. Either split up the patch along those changes (if possible)
or improve the commit message to explain those changes.
Regards,
Lucas
> + if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> + regulator_disable(imx6_pcie->vpcie);
> return ret;
> }
>
> 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;
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump
@ 2021-09-08 8:42 ` Lucas Stach
0 siblings, 0 replies; 25+ messages in thread
From: Lucas Stach @ 2021-09-08 8:42 UTC (permalink / raw)
To: Richard Zhu, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> Since there is error return check of the host_init callback, add error
> check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> function.
>
> Because that i.MX PCIe doesn't support the hot-plug feature. To save
> power consumption as much as possible, turn off the clocks and power
> supplies when the PCIe PHY link is never came up in probe procedure.
>
> When PCIe link is never came up and vpcie regulator is present, there
> would be following dump when try to put the regulator.
> Disable this regulator to fix this dump when link is never came up.
>
> [ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
> [ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
> [ 2.348160] ------------[ cut here ]------------
> [ 2.352778] WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
> [ 2.362184] Modules linked in:
> [ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> [ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT)
> [ 2.380040] Workqueue: events_unbound async_run_entry_fn
> [ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> [ 2.391369] pc : _regulator_put.part.0+0x14c/0x158
> [ 2.396163] lr : regulator_put+0x34/0x48
> [ 2.400088] sp : ffff8000122ebb30
> [ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000
> [ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc
> [ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18
> [ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010
> [ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108
> [ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000
> [ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540
> [ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8
> [ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30
> [ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001
> [ 2.474847] Call trace:
> [ 2.477295] _regulator_put.part.0+0x14c/0x158
> [ 2.481742] regulator_put+0x34/0x48
> [ 2.485322] devm_regulator_release+0x10/0x18
> [ 2.489681] release_nodes+0x38/0x60
> [ 2.493262] devres_release_all+0x88/0xd0
> [ 2.497276] really_probe+0xd0/0x2e8
> [ 2.500858] __driver_probe_device+0x74/0xd8
> [ 2.505137] driver_probe_device+0x7c/0x108
> [ 2.509325] __device_attach_driver+0x8c/0xd0
> [ 2.513685] bus_for_each_drv+0x74/0xc0
> [ 2.517531] __device_attach_async_helper+0xb4/0xd8
> [ 2.522419] async_run_entry_fn+0x30/0x100
> [ 2.526521] process_one_work+0x19c/0x320
> [ 2.530532] worker_thread+0x48/0x418
> [ 2.534199] kthread+0x14c/0x158
> [ 2.537432] ret_from_fork+0x10/0x18
> [ 2.541013] ---[ end trace 3664ca4a50ce849b ]---
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 28 +++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 0264432e4c4a..129928e42f84 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -144,6 +144,7 @@ struct imx6_pcie {
> #define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
>
> static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
>
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> {
> @@ -485,24 +486,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;
Why do we need a separate variable for the error code here? Why not
just use the existing 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;
> + 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;
> }
>
> @@ -557,7 +558,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) {
> @@ -566,6 +567,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)
> @@ -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> imx6_pcie_reset_phy(imx6_pcie);
> + imx6_pcie_clk_disable(imx6_pcie);
This is a separate fix for the clock enable counts, that isn't
mentioned in the commit message.
It seems like this patch fixes in fact 3 issues: error propagation from
host_init, fix regulator reference handling, fix clk reference
handling. Either split up the patch along those changes (if possible)
or improve the commit message to explain those changes.
Regards,
Lucas
> + if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> + regulator_disable(imx6_pcie->vpcie);
> return ret;
> }
>
> 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;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump
2021-09-08 8:42 ` Lucas Stach
@ 2021-09-08 8:59 ` Richard Zhu
-1 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 8:59 UTC (permalink / raw)
To: Lucas Stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, dl-linux-imx, linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Wednesday, September 8, 2021 4:42 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator
> dump
>
> Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> > Since there is error return check of the host_init callback, add error
> > check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > function.
> >
> > Because that i.MX PCIe doesn't support the hot-plug feature. To save
> > power consumption as much as possible, turn off the clocks and power
> > supplies when the PCIe PHY link is never came up in probe procedure.
> >
> > When PCIe link is never came up and vpcie regulator is present, there
> > would be following dump when try to put the regulator.
> > Disable this regulator to fix this dump when link is never came up.
> >
> > [ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
> > [ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
> > [ 2.348160] ------------[ cut here ]------------
> > [ 2.352778] WARNING: CPU: 3 PID: 119 at
> drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
> > [ 2.362184] Modules linked in:
> > [ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted
> 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> > [ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT)
> > [ 2.380040] Workqueue: events_unbound async_run_entry_fn
> > [ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > [ 2.391369] pc : _regulator_put.part.0+0x14c/0x158
> > [ 2.396163] lr : regulator_put+0x34/0x48
> > [ 2.400088] sp : ffff8000122ebb30
> > [ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27:
> 0000000000000000
> > [ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24:
> ffff00000025f2bc
> > [ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21:
> ffff8000122ebc18
> > [ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18:
> 0000000000000010
> > [ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15:
> 0000000000000108
> > [ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12:
> 0000000000000000
> > [ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 :
> ffff80001076a540
> > [ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 :
> ffff000000176cd8
> > [ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 :
> ffff800011e3fe30
> > [ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 :
> 0000000000000001
> > [ 2.474847] Call trace:
> > [ 2.477295] _regulator_put.part.0+0x14c/0x158
> > [ 2.481742] regulator_put+0x34/0x48
> > [ 2.485322] devm_regulator_release+0x10/0x18
> > [ 2.489681] release_nodes+0x38/0x60
> > [ 2.493262] devres_release_all+0x88/0xd0
> > [ 2.497276] really_probe+0xd0/0x2e8
> > [ 2.500858] __driver_probe_device+0x74/0xd8
> > [ 2.505137] driver_probe_device+0x7c/0x108
> > [ 2.509325] __device_attach_driver+0x8c/0xd0
> > [ 2.513685] bus_for_each_drv+0x74/0xc0
> > [ 2.517531] __device_attach_async_helper+0xb4/0xd8
> > [ 2.522419] async_run_entry_fn+0x30/0x100
> > [ 2.526521] process_one_work+0x19c/0x320
> > [ 2.530532] worker_thread+0x48/0x418
> > [ 2.534199] kthread+0x14c/0x158
> > [ 2.537432] ret_from_fork+0x10/0x18
> > [ 2.541013] ---[ end trace 3664ca4a50ce849b ]---
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 28
> > +++++++++++++++++++--------
> > 1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 0264432e4c4a..129928e42f84 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -144,6 +144,7 @@ struct imx6_pcie {
> > #define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
> >
> > static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> > +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
> >
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val) { @@ -485,24 +486,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;
>
> Why do we need a separate variable for the error code here? Why not just
> use the existing ret?
[Richard Zhu] Thanks for your review.
The "ret" variable might be assigned to one "0" by regulator_disable() in the error exit.
So, I use another one "err" variable to avoid this confliction.
How about do not make the err check of regulator_enable/disable, and just use the "ret" for the error code?
if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
ret = regulator_disable(imx6_pcie->vpcie);
if (ret)
dev_err(dev, "failed to disable vpcie regulator: %d\n",
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;
> > + 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;
> > }
> >
> > @@ -557,7 +558,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)
> > { @@ -566,6 +567,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) @@
> > -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > imx6_pcie_reset_phy(imx6_pcie);
> > + imx6_pcie_clk_disable(imx6_pcie);
>
> This is a separate fix for the clock enable counts, that isn't mentioned in the
> commit message.
>
> It seems like this patch fixes in fact 3 issues: error propagation from host_init,
> fix regulator reference handling, fix clk reference handling. Either split up the
> patch along those changes (if possible) or improve the commit message to
> explain those changes.
[Richard Zhu] My original purpose of this patch is to turn off the clocks
and power supplies( regulators and so on.) when link is down(for example, no EP device connected).
So, I combine the fix of the clock/regulator used counter balance together.
Would improve the commit message to explain these changes later.
You're right, the error propagation from host_init should be separated to another patch.
Thanks a lot 😊.
BR
Richard
>
> Regards,
> Lucas
>
> > + if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > + regulator_disable(imx6_pcie->vpcie);
> > return ret;
> > }
> >
> > 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;
>
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump
@ 2021-09-08 8:59 ` Richard Zhu
0 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 8:59 UTC (permalink / raw)
To: Lucas Stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, dl-linux-imx, linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Wednesday, September 8, 2021 4:42 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator
> dump
>
> Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> > Since there is error return check of the host_init callback, add error
> > check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > function.
> >
> > Because that i.MX PCIe doesn't support the hot-plug feature. To save
> > power consumption as much as possible, turn off the clocks and power
> > supplies when the PCIe PHY link is never came up in probe procedure.
> >
> > When PCIe link is never came up and vpcie regulator is present, there
> > would be following dump when try to put the regulator.
> > Disable this regulator to fix this dump when link is never came up.
> >
> > [ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
> > [ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
> > [ 2.348160] ------------[ cut here ]------------
> > [ 2.352778] WARNING: CPU: 3 PID: 119 at
> drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158
> > [ 2.362184] Modules linked in:
> > [ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted
> 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10
> > [ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT)
> > [ 2.380040] Workqueue: events_unbound async_run_entry_fn
> > [ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > [ 2.391369] pc : _regulator_put.part.0+0x14c/0x158
> > [ 2.396163] lr : regulator_put+0x34/0x48
> > [ 2.400088] sp : ffff8000122ebb30
> > [ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27:
> 0000000000000000
> > [ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24:
> ffff00000025f2bc
> > [ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21:
> ffff8000122ebc18
> > [ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18:
> 0000000000000010
> > [ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15:
> 0000000000000108
> > [ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12:
> 0000000000000000
> > [ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 :
> ffff80001076a540
> > [ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 :
> ffff000000176cd8
> > [ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 :
> ffff800011e3fe30
> > [ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 :
> 0000000000000001
> > [ 2.474847] Call trace:
> > [ 2.477295] _regulator_put.part.0+0x14c/0x158
> > [ 2.481742] regulator_put+0x34/0x48
> > [ 2.485322] devm_regulator_release+0x10/0x18
> > [ 2.489681] release_nodes+0x38/0x60
> > [ 2.493262] devres_release_all+0x88/0xd0
> > [ 2.497276] really_probe+0xd0/0x2e8
> > [ 2.500858] __driver_probe_device+0x74/0xd8
> > [ 2.505137] driver_probe_device+0x7c/0x108
> > [ 2.509325] __device_attach_driver+0x8c/0xd0
> > [ 2.513685] bus_for_each_drv+0x74/0xc0
> > [ 2.517531] __device_attach_async_helper+0xb4/0xd8
> > [ 2.522419] async_run_entry_fn+0x30/0x100
> > [ 2.526521] process_one_work+0x19c/0x320
> > [ 2.530532] worker_thread+0x48/0x418
> > [ 2.534199] kthread+0x14c/0x158
> > [ 2.537432] ret_from_fork+0x10/0x18
> > [ 2.541013] ---[ end trace 3664ca4a50ce849b ]---
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 28
> > +++++++++++++++++++--------
> > 1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 0264432e4c4a..129928e42f84 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -144,6 +144,7 @@ struct imx6_pcie {
> > #define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3)
> >
> > static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> > +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
> >
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val) { @@ -485,24 +486,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;
>
> Why do we need a separate variable for the error code here? Why not just
> use the existing ret?
[Richard Zhu] Thanks for your review.
The "ret" variable might be assigned to one "0" by regulator_disable() in the error exit.
So, I use another one "err" variable to avoid this confliction.
How about do not make the err check of regulator_enable/disable, and just use the "ret" for the error code?
if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
ret = regulator_disable(imx6_pcie->vpcie);
if (ret)
dev_err(dev, "failed to disable vpcie regulator: %d\n",
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;
> > + 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;
> > }
> >
> > @@ -557,7 +558,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)
> > { @@ -566,6 +567,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) @@
> > -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > imx6_pcie_reset_phy(imx6_pcie);
> > + imx6_pcie_clk_disable(imx6_pcie);
>
> This is a separate fix for the clock enable counts, that isn't mentioned in the
> commit message.
>
> It seems like this patch fixes in fact 3 issues: error propagation from host_init,
> fix regulator reference handling, fix clk reference handling. Either split up the
> patch along those changes (if possible) or improve the commit message to
> explain those changes.
[Richard Zhu] My original purpose of this patch is to turn off the clocks
and power supplies( regulators and so on.) when link is down(for example, no EP device connected).
So, I combine the fix of the clock/regulator used counter balance together.
Would improve the commit message to explain these changes later.
You're right, the error propagation from host_init should be separated to another patch.
Thanks a lot 😊.
BR
Richard
>
> Regards,
> Lucas
>
> > + if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > + regulator_disable(imx6_pcie->vpcie);
> > return ret;
> > }
> >
> > 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;
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/3] PCI: imx: add compliance tests mode to enable measure signal quality
2021-09-08 6:59 ` Richard Zhu
@ 2021-09-08 6:59 ` Richard Zhu
-1 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 6:59 UTC (permalink / raw)
To: l.stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel,
Richard Zhu
Refer to the system board signal Quality of PCIe archiecture PHY test
specification. Signal quality tests 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 during the compliance tests although
the PHY link might be down.
Add the i.MX PCIe compliance tests mode enable option to keep the and
powers on, and finish the driver probe without error return.
Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
compliance tests mode.
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 41 +++++++++++++++++++++------
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 129928e42f84..3aef0e86f1c2 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -143,6 +143,7 @@ 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 int imx6_pcie_cmp_enabled;
static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
@@ -748,10 +749,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_enabled) {
+ 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);
@@ -812,9 +815,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
imx6_pcie_reset_phy(imx6_pcie);
- imx6_pcie_clk_disable(imx6_pcie);
- if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
- regulator_disable(imx6_pcie->vpcie);
+ if (!imx6_pcie_cmp_enabled) {
+ imx6_pcie_clk_disable(imx6_pcie);
+ if (imx6_pcie->vpcie
+ && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+ regulator_disable(imx6_pcie->vpcie);
+ }
return ret;
}
@@ -1010,6 +1016,17 @@ static const struct dev_pm_ops imx6_pcie_pm_ops = {
imx6_pcie_resume_noirq)
};
+static int __init imx6_pcie_compliance_test_enable(char *str)
+{
+ if (!strcmp(str, "yes")) {
+ pr_info("Enable the i.MX PCIe TX/CLK compliance tests mode.\n");
+ imx6_pcie_cmp_enabled = 1;
+ }
+ return 1;
+}
+
+__setup("pcie_cmp_enabled=", imx6_pcie_compliance_test_enable);
+
static int imx6_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1187,8 +1204,16 @@ 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_enabled) {
+ /* The PCIE clocks and powers wouldn't be turned off */
+ dev_info(dev, "To do the compliance tests.\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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 3/3] PCI: imx: add compliance tests mode to enable measure signal quality
@ 2021-09-08 6:59 ` Richard Zhu
0 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 6:59 UTC (permalink / raw)
To: l.stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel,
Richard Zhu
Refer to the system board signal Quality of PCIe archiecture PHY test
specification. Signal quality tests 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 during the compliance tests although
the PHY link might be down.
Add the i.MX PCIe compliance tests mode enable option to keep the and
powers on, and finish the driver probe without error return.
Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
compliance tests mode.
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 41 +++++++++++++++++++++------
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 129928e42f84..3aef0e86f1c2 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -143,6 +143,7 @@ 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 int imx6_pcie_cmp_enabled;
static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
@@ -748,10 +749,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_enabled) {
+ 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);
@@ -812,9 +815,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
imx6_pcie_reset_phy(imx6_pcie);
- imx6_pcie_clk_disable(imx6_pcie);
- if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
- regulator_disable(imx6_pcie->vpcie);
+ if (!imx6_pcie_cmp_enabled) {
+ imx6_pcie_clk_disable(imx6_pcie);
+ if (imx6_pcie->vpcie
+ && regulator_is_enabled(imx6_pcie->vpcie) > 0)
+ regulator_disable(imx6_pcie->vpcie);
+ }
return ret;
}
@@ -1010,6 +1016,17 @@ static const struct dev_pm_ops imx6_pcie_pm_ops = {
imx6_pcie_resume_noirq)
};
+static int __init imx6_pcie_compliance_test_enable(char *str)
+{
+ if (!strcmp(str, "yes")) {
+ pr_info("Enable the i.MX PCIe TX/CLK compliance tests mode.\n");
+ imx6_pcie_cmp_enabled = 1;
+ }
+ return 1;
+}
+
+__setup("pcie_cmp_enabled=", imx6_pcie_compliance_test_enable);
+
static int imx6_pcie_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1187,8 +1204,16 @@ 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_enabled) {
+ /* The PCIE clocks and powers wouldn't be turned off */
+ dev_info(dev, "To do the compliance tests.\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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 3/3] PCI: imx: add compliance tests mode to enable measure signal quality
2021-09-08 6:59 ` Richard Zhu
@ 2021-09-08 8:34 ` Lucas Stach
-1 siblings, 0 replies; 25+ messages in thread
From: Lucas Stach @ 2021-09-08 8:34 UTC (permalink / raw)
To: Richard Zhu, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel
Hi Richard,
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> Refer to the system board signal Quality of PCIe archiecture PHY test
> specification. Signal quality tests 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 during the compliance tests although
> the PHY link might be down.
> Add the i.MX PCIe compliance tests mode enable option to keep the and
> powers on, and finish the driver probe without error return.
>
> Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
> compliance tests mode.
Adding "random" kernel command line options isn't going to fly. If at
all, this should be a module_param so it gets properly namespaced. Also
this needs a more descriptive name, right now this is abbreviating the
one thing that would tell a user what this is about: compliance
testing.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 41 +++++++++++++++++++++------
> 1 file changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 129928e42f84..3aef0e86f1c2 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -143,6 +143,7 @@ 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 int imx6_pcie_cmp_enabled;
> static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
>
> @@ -748,10 +749,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_enabled) {
> + 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);
> @@ -812,9 +815,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> imx6_pcie_reset_phy(imx6_pcie);
> - imx6_pcie_clk_disable(imx6_pcie);
> - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> - regulator_disable(imx6_pcie->vpcie);
> + if (!imx6_pcie_cmp_enabled) {
> + imx6_pcie_clk_disable(imx6_pcie);
> + if (imx6_pcie->vpcie
> + && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> + regulator_disable(imx6_pcie->vpcie);
> + }
> return ret;
> }
>
> @@ -1010,6 +1016,17 @@ static const struct dev_pm_ops imx6_pcie_pm_ops = {
> imx6_pcie_resume_noirq)
> };
>
> +static int __init imx6_pcie_compliance_test_enable(char *str)
> +{
> + if (!strcmp(str, "yes")) {
> + pr_info("Enable the i.MX PCIe TX/CLK compliance tests mode.\n");
> + imx6_pcie_cmp_enabled = 1;
> + }
> + return 1;
> +}
> +
> +__setup("pcie_cmp_enabled=", imx6_pcie_compliance_test_enable);
> +
> static int imx6_pcie_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1187,8 +1204,16 @@ 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_enabled) {
> + /* The PCIE clocks and powers wouldn't be turned off */
> + dev_info(dev, "To do the compliance tests.\n");
This needs a better message, like "Driver loaded with compliance test
mode enabled" and the message needs to be shown unconditionally, not
only when the host init fails. We don't want to have the user track
down weird issues when the compliance test option was specified by
accident and the link is almost working due to this.
Regards,
Lucas
> + 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);
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 3/3] PCI: imx: add compliance tests mode to enable measure signal quality
@ 2021-09-08 8:34 ` Lucas Stach
0 siblings, 0 replies; 25+ messages in thread
From: Lucas Stach @ 2021-09-08 8:34 UTC (permalink / raw)
To: Richard Zhu, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel
Hi Richard,
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> Refer to the system board signal Quality of PCIe archiecture PHY test
> specification. Signal quality tests 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 during the compliance tests although
> the PHY link might be down.
> Add the i.MX PCIe compliance tests mode enable option to keep the and
> powers on, and finish the driver probe without error return.
>
> Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
> compliance tests mode.
Adding "random" kernel command line options isn't going to fly. If at
all, this should be a module_param so it gets properly namespaced. Also
this needs a more descriptive name, right now this is abbreviating the
one thing that would tell a user what this is about: compliance
testing.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 41 +++++++++++++++++++++------
> 1 file changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 129928e42f84..3aef0e86f1c2 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -143,6 +143,7 @@ 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 int imx6_pcie_cmp_enabled;
> static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
>
> @@ -748,10 +749,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_enabled) {
> + 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);
> @@ -812,9 +815,12 @@ static int imx6_pcie_start_link(struct dw_pcie *pci)
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> imx6_pcie_reset_phy(imx6_pcie);
> - imx6_pcie_clk_disable(imx6_pcie);
> - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> - regulator_disable(imx6_pcie->vpcie);
> + if (!imx6_pcie_cmp_enabled) {
> + imx6_pcie_clk_disable(imx6_pcie);
> + if (imx6_pcie->vpcie
> + && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> + regulator_disable(imx6_pcie->vpcie);
> + }
> return ret;
> }
>
> @@ -1010,6 +1016,17 @@ static const struct dev_pm_ops imx6_pcie_pm_ops = {
> imx6_pcie_resume_noirq)
> };
>
> +static int __init imx6_pcie_compliance_test_enable(char *str)
> +{
> + if (!strcmp(str, "yes")) {
> + pr_info("Enable the i.MX PCIe TX/CLK compliance tests mode.\n");
> + imx6_pcie_cmp_enabled = 1;
> + }
> + return 1;
> +}
> +
> +__setup("pcie_cmp_enabled=", imx6_pcie_compliance_test_enable);
> +
> static int imx6_pcie_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1187,8 +1204,16 @@ 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_enabled) {
> + /* The PCIE clocks and powers wouldn't be turned off */
> + dev_info(dev, "To do the compliance tests.\n");
This needs a better message, like "Driver loaded with compliance test
mode enabled" and the message needs to be shown unconditionally, not
only when the host init fails. We don't want to have the user track
down weird issues when the compliance test option was specified by
accident and the link is almost working due to this.
Regards,
Lucas
> + 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);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 3/3] PCI: imx: add compliance tests mode to enable measure signal quality
2021-09-08 8:34 ` Lucas Stach
@ 2021-09-08 8:46 ` Richard Zhu
-1 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 8:46 UTC (permalink / raw)
To: Lucas Stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, dl-linux-imx, linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Wednesday, September 8, 2021 4:34 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH 3/3] PCI: imx: add compliance tests mode to enable
> measure signal quality
>
> Hi Richard,
>
> Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> > Refer to the system board signal Quality of PCIe archiecture PHY test
> > specification. Signal quality tests 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 during the compliance tests although
> > the PHY link might be down.
> > Add the i.MX PCIe compliance tests mode enable option to keep the and
> > powers on, and finish the driver probe without error return.
> >
> > Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
> > compliance tests mode.
>
> Adding "random" kernel command line options isn't going to fly. If at all, this
> should be a module_param so it gets properly namespaced. Also this needs a
> more descriptive name, right now this is abbreviating the one thing that would
> tell a user what this is about: compliance testing.
>
[Richard Zhu] Thanks a lot for your review comments.
Okay, I would use the module_param replace the command line option.
And Would add more compliance tests descriptions(what's compliance used for,
and the simple scope of the tests) later.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 41
> > +++++++++++++++++++++------
> > 1 file changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 129928e42f84..3aef0e86f1c2 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -143,6 +143,7 @@ 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 int imx6_pcie_cmp_enabled;
> > static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie); static
> > void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
> >
> > @@ -748,10 +749,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_enabled) {
> > + 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);
> > @@ -812,9 +815,12 @@ static int imx6_pcie_start_link(struct dw_pcie
> *pci)
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > imx6_pcie_reset_phy(imx6_pcie);
> > - imx6_pcie_clk_disable(imx6_pcie);
> > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > - regulator_disable(imx6_pcie->vpcie);
> > + if (!imx6_pcie_cmp_enabled) {
> > + imx6_pcie_clk_disable(imx6_pcie);
> > + if (imx6_pcie->vpcie
> > + && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > + regulator_disable(imx6_pcie->vpcie);
> > + }
> > return ret;
> > }
> >
> > @@ -1010,6 +1016,17 @@ static const struct dev_pm_ops
> imx6_pcie_pm_ops = {
> > imx6_pcie_resume_noirq)
> > };
> >
> > +static int __init imx6_pcie_compliance_test_enable(char *str) {
> > + if (!strcmp(str, "yes")) {
> > + pr_info("Enable the i.MX PCIe TX/CLK compliance tests mode.\n");
> > + imx6_pcie_cmp_enabled = 1;
> > + }
> > + return 1;
> > +}
> > +
> > +__setup("pcie_cmp_enabled=", imx6_pcie_compliance_test_enable);
> > +
> > static int imx6_pcie_probe(struct platform_device *pdev) {
> > struct device *dev = &pdev->dev;
> > @@ -1187,8 +1204,16 @@ 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_enabled) {
> > + /* The PCIE clocks and powers wouldn't be turned off */
> > + dev_info(dev, "To do the compliance tests.\n");
>
> This needs a better message, like "Driver loaded with compliance test mode
> enabled" and the message needs to be shown unconditionally, not only when
> the host init fails. We don't want to have the user track down weird issues
> when the compliance test option was specified by accident and the link is
> almost working due to this.
>
[Richard Zhu] Thanks. Ok, got that, would update the message in next version.
Best Regards
Richard Zhu
> Regards,
> Lucas
>
> > + 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);
>
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 3/3] PCI: imx: add compliance tests mode to enable measure signal quality
@ 2021-09-08 8:46 ` Richard Zhu
0 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 8:46 UTC (permalink / raw)
To: Lucas Stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, dl-linux-imx, linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Wednesday, September 8, 2021 4:34 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH 3/3] PCI: imx: add compliance tests mode to enable
> measure signal quality
>
> Hi Richard,
>
> Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> > Refer to the system board signal Quality of PCIe archiecture PHY test
> > specification. Signal quality tests 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 during the compliance tests although
> > the PHY link might be down.
> > Add the i.MX PCIe compliance tests mode enable option to keep the and
> > powers on, and finish the driver probe without error return.
> >
> > Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
> > compliance tests mode.
>
> Adding "random" kernel command line options isn't going to fly. If at all, this
> should be a module_param so it gets properly namespaced. Also this needs a
> more descriptive name, right now this is abbreviating the one thing that would
> tell a user what this is about: compliance testing.
>
[Richard Zhu] Thanks a lot for your review comments.
Okay, I would use the module_param replace the command line option.
And Would add more compliance tests descriptions(what's compliance used for,
and the simple scope of the tests) later.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 41
> > +++++++++++++++++++++------
> > 1 file changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 129928e42f84..3aef0e86f1c2 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -143,6 +143,7 @@ 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 int imx6_pcie_cmp_enabled;
> > static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie); static
> > void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie);
> >
> > @@ -748,10 +749,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_enabled) {
> > + 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);
> > @@ -812,9 +815,12 @@ static int imx6_pcie_start_link(struct dw_pcie
> *pci)
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> > imx6_pcie_reset_phy(imx6_pcie);
> > - imx6_pcie_clk_disable(imx6_pcie);
> > - if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > - regulator_disable(imx6_pcie->vpcie);
> > + if (!imx6_pcie_cmp_enabled) {
> > + imx6_pcie_clk_disable(imx6_pcie);
> > + if (imx6_pcie->vpcie
> > + && regulator_is_enabled(imx6_pcie->vpcie) > 0)
> > + regulator_disable(imx6_pcie->vpcie);
> > + }
> > return ret;
> > }
> >
> > @@ -1010,6 +1016,17 @@ static const struct dev_pm_ops
> imx6_pcie_pm_ops = {
> > imx6_pcie_resume_noirq)
> > };
> >
> > +static int __init imx6_pcie_compliance_test_enable(char *str) {
> > + if (!strcmp(str, "yes")) {
> > + pr_info("Enable the i.MX PCIe TX/CLK compliance tests mode.\n");
> > + imx6_pcie_cmp_enabled = 1;
> > + }
> > + return 1;
> > +}
> > +
> > +__setup("pcie_cmp_enabled=", imx6_pcie_compliance_test_enable);
> > +
> > static int imx6_pcie_probe(struct platform_device *pdev) {
> > struct device *dev = &pdev->dev;
> > @@ -1187,8 +1204,16 @@ 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_enabled) {
> > + /* The PCIE clocks and powers wouldn't be turned off */
> > + dev_info(dev, "To do the compliance tests.\n");
>
> This needs a better message, like "Driver loaded with compliance test mode
> enabled" and the message needs to be shown unconditionally, not only when
> the host init fails. We don't want to have the user track down weird issues
> when the compliance test option was specified by accident and the link is
> almost working due to this.
>
[Richard Zhu] Thanks. Ok, got that, would update the message in next version.
Best Regards
Richard Zhu
> Regards,
> Lucas
>
> > + 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);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
2021-09-08 6:59 ` Richard Zhu
@ 2021-09-08 8:47 ` Lucas Stach
-1 siblings, 0 replies; 25+ messages in thread
From: Lucas Stach @ 2021-09-08 8:47 UTC (permalink / raw)
To: Richard Zhu, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> No function changes, just encapsulate the i.MX PCIe clocks enable
> operations into one standalone function
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 82 +++++++++++++++++----------
> 1 file changed, 51 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 80fc98acf097..0264432e4c4a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> +
I don't think this is strictly needed. Can you just move the placement
of the new imx6_pcie_clk_enable function in the file, such that we can
avoid the forward declaration?
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> @@ -498,33 +500,12 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> }
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_phy clock\n");
> - goto err_pcie_phy;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + ret = imx6_pcie_clk_enable(imx6_pcie);
> if (ret) {
> - dev_err(dev, "unable to enable pcie_bus clock\n");
> - goto err_pcie_bus;
> + dev_err(dev, "unable to enable pcie clocks\n");
> + goto err_clks;
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie clock\n");
> - goto err_pcie;
> - }
> -
> - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie ref clock\n");
> - goto err_ref_clk;
> - }
> -
> - /* allow the clocks to stabilize */
> - usleep_range(200, 500);
> -
> /* Some boards don't have PCIe reset GPIO. */
> if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> @@ -578,13 +559,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)
> @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> usleep_range(1000, 10000);
> }
>
> +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;
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_phy clock\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_bus clock\n");
> + goto err_pcie_bus;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie clock\n");
> + goto err_pcie;
> + }
> +
> + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie ref clock\n");
> + goto err_ref_clk;
> + }
> +
> + /* 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;
> +
Superfluous newline.
Regards,
Lucas
> +}
> +
> static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> {
> clk_disable_unprepare(imx6_pcie->pcie);
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
@ 2021-09-08 8:47 ` Lucas Stach
0 siblings, 0 replies; 25+ messages in thread
From: Lucas Stach @ 2021-09-08 8:47 UTC (permalink / raw)
To: Richard Zhu, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, linux-imx, linux-arm-kernel, linux-kernel, kernel
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> No function changes, just encapsulate the i.MX PCIe clocks enable
> operations into one standalone function
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 82 +++++++++++++++++----------
> 1 file changed, 51 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 80fc98acf097..0264432e4c4a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> +
I don't think this is strictly needed. Can you just move the placement
of the new imx6_pcie_clk_enable function in the file, such that we can
avoid the forward declaration?
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> @@ -498,33 +500,12 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> }
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_phy clock\n");
> - goto err_pcie_phy;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + ret = imx6_pcie_clk_enable(imx6_pcie);
> if (ret) {
> - dev_err(dev, "unable to enable pcie_bus clock\n");
> - goto err_pcie_bus;
> + dev_err(dev, "unable to enable pcie clocks\n");
> + goto err_clks;
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie clock\n");
> - goto err_pcie;
> - }
> -
> - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie ref clock\n");
> - goto err_ref_clk;
> - }
> -
> - /* allow the clocks to stabilize */
> - usleep_range(200, 500);
> -
> /* Some boards don't have PCIe reset GPIO. */
> if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> @@ -578,13 +559,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)
> @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> usleep_range(1000, 10000);
> }
>
> +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;
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_phy clock\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_bus clock\n");
> + goto err_pcie_bus;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie clock\n");
> + goto err_pcie;
> + }
> +
> + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie ref clock\n");
> + goto err_ref_clk;
> + }
> +
> + /* 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;
> +
Superfluous newline.
Regards,
Lucas
> +}
> +
> static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> {
> clk_disable_unprepare(imx6_pcie->pcie);
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
2021-09-08 8:47 ` Lucas Stach
@ 2021-09-08 9:02 ` Richard Zhu
-1 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 9:02 UTC (permalink / raw)
To: Lucas Stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, dl-linux-imx, linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Wednesday, September 8, 2021 4:48 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one
> standalone function
>
> Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> > No function changes, just encapsulate the i.MX PCIe clocks enable
> > operations into one standalone function
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 82
> > +++++++++++++++++----------
> > 1 file changed, 51 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 80fc98acf097..0264432e4c4a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> > +
> I don't think this is strictly needed. Can you just move the placement of the
> new imx6_pcie_clk_enable function in the file, such that we can avoid the
> forward declaration?
>
[Richard Zhu] Thanks for your quickly review.
Okay, would move the placement to avoid he forward declaration.
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val) {
> > struct dw_pcie *pci = imx6_pcie->pci; @@ -498,33 +500,12 @@ static
> > void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > }
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_phy clock\n");
> > - goto err_pcie_phy;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + ret = imx6_pcie_clk_enable(imx6_pcie);
> > if (ret) {
> > - dev_err(dev, "unable to enable pcie_bus clock\n");
> > - goto err_pcie_bus;
> > + dev_err(dev, "unable to enable pcie clocks\n");
> > + goto err_clks;
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie clock\n");
> > - goto err_pcie;
> > - }
> > -
> > - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie ref clock\n");
> > - goto err_ref_clk;
> > - }
> > -
> > - /* allow the clocks to stabilize */
> > - usleep_range(200, 500);
> > -
> > /* Some boards don't have PCIe reset GPIO. */
> > if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > @@ -578,13 +559,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)
> > @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> > usleep_range(1000, 10000);
> > }
> >
> > +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;
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_phy clock\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_bus clock\n");
> > + goto err_pcie_bus;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie clock\n");
> > + goto err_pcie;
> > + }
> > +
> > + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie ref clock\n");
> > + goto err_ref_clk;
> > + }
> > +
> > + /* 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;
> > +
> Superfluous newline.
[Richard Zhu]Got that. Blank line would be removed later.
Thanks.
>
> Regards,
> Lucas
>
> > +}
> > +
> > static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) {
> > clk_disable_unprepare(imx6_pcie->pcie);
>
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
@ 2021-09-08 9:02 ` Richard Zhu
0 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-08 9:02 UTC (permalink / raw)
To: Lucas Stach, bhelgaas, lorenzo.pieralisi
Cc: linux-pci, dl-linux-imx, linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: Wednesday, September 8, 2021 4:48 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com
> Cc: linux-pci@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one
> standalone function
>
> Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
> > No function changes, just encapsulate the i.MX PCIe clocks enable
> > operations into one standalone function
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 82
> > +++++++++++++++++----------
> > 1 file changed, 51 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 80fc98acf097..0264432e4c4a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> > +
> I don't think this is strictly needed. Can you just move the placement of the
> new imx6_pcie_clk_enable function in the file, such that we can avoid the
> forward declaration?
>
[Richard Zhu] Thanks for your quickly review.
Okay, would move the placement to avoid he forward declaration.
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val) {
> > struct dw_pcie *pci = imx6_pcie->pci; @@ -498,33 +500,12 @@ static
> > void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > }
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_phy clock\n");
> > - goto err_pcie_phy;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + ret = imx6_pcie_clk_enable(imx6_pcie);
> > if (ret) {
> > - dev_err(dev, "unable to enable pcie_bus clock\n");
> > - goto err_pcie_bus;
> > + dev_err(dev, "unable to enable pcie clocks\n");
> > + goto err_clks;
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie clock\n");
> > - goto err_pcie;
> > - }
> > -
> > - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie ref clock\n");
> > - goto err_ref_clk;
> > - }
> > -
> > - /* allow the clocks to stabilize */
> > - usleep_range(200, 500);
> > -
> > /* Some boards don't have PCIe reset GPIO. */
> > if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > @@ -578,13 +559,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)
> > @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> > usleep_range(1000, 10000);
> > }
> >
> > +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;
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_phy clock\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_bus clock\n");
> > + goto err_pcie_bus;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie clock\n");
> > + goto err_pcie;
> > + }
> > +
> > + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie ref clock\n");
> > + goto err_ref_clk;
> > + }
> > +
> > + /* 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;
> > +
> Superfluous newline.
[Richard Zhu]Got that. Blank line would be removed later.
Thanks.
>
> Regards,
> Lucas
>
> > +}
> > +
> > static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) {
> > clk_disable_unprepare(imx6_pcie->pcie);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
2021-09-08 6:59 ` Richard Zhu
(?)
@ 2021-09-08 13:41 ` kernel test robot
-1 siblings, 0 replies; 25+ messages in thread
From: kernel test robot @ 2021-09-08 13:41 UTC (permalink / raw)
To: Richard Zhu, l.stach, bhelgaas, lorenzo.pieralisi
Cc: kbuild-all, linux-pci, linux-imx, linux-arm-kernel, linux-kernel,
kernel, Richard Zhu
[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]
Hi Richard,
I love your patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v5.14]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Richard-Zhu/PCI-imx-encapsulate-the-clock-enable-into-one-standalone-function/20210908-152610
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3872c55dc0f8d93641fd342d0524fbcac873de49
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Richard-Zhu/PCI-imx-encapsulate-the-clock-enable-into-one-standalone-function/20210908-152610
git checkout 3872c55dc0f8d93641fd342d0524fbcac873de49
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ia64-linux-ld: drivers/pci/controller/dwc/pci-imx6.o: in function `imx6_pcie_deassert_core_reset':
>> pci-imx6.c:(.text+0x17c2): undefined reference to `imx6_pcie_clk_enable'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69781 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
@ 2021-09-08 13:41 ` kernel test robot
0 siblings, 0 replies; 25+ messages in thread
From: kernel test robot @ 2021-09-08 13:41 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1821 bytes --]
Hi Richard,
I love your patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v5.14]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Richard-Zhu/PCI-imx-encapsulate-the-clock-enable-into-one-standalone-function/20210908-152610
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3872c55dc0f8d93641fd342d0524fbcac873de49
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Richard-Zhu/PCI-imx-encapsulate-the-clock-enable-into-one-standalone-function/20210908-152610
git checkout 3872c55dc0f8d93641fd342d0524fbcac873de49
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ia64-linux-ld: drivers/pci/controller/dwc/pci-imx6.o: in function `imx6_pcie_deassert_core_reset':
>> pci-imx6.c:(.text+0x17c2): undefined reference to `imx6_pcie_clk_enable'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 69781 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
@ 2021-09-08 13:41 ` kernel test robot
0 siblings, 0 replies; 25+ messages in thread
From: kernel test robot @ 2021-09-08 13:41 UTC (permalink / raw)
To: Richard Zhu, l.stach, bhelgaas, lorenzo.pieralisi
Cc: kbuild-all, linux-pci, linux-imx, linux-arm-kernel, linux-kernel,
kernel, Richard Zhu
[-- Attachment #1: Type: text/plain, Size: 1783 bytes --]
Hi Richard,
I love your patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v5.14]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Richard-Zhu/PCI-imx-encapsulate-the-clock-enable-into-one-standalone-function/20210908-152610
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: ia64-allyesconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3872c55dc0f8d93641fd342d0524fbcac873de49
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Richard-Zhu/PCI-imx-encapsulate-the-clock-enable-into-one-standalone-function/20210908-152610
git checkout 3872c55dc0f8d93641fd342d0524fbcac873de49
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ia64-linux-ld: drivers/pci/controller/dwc/pci-imx6.o: in function `imx6_pcie_deassert_core_reset':
>> pci-imx6.c:(.text+0x17c2): undefined reference to `imx6_pcie_clk_enable'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69781 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
2021-09-08 6:59 ` Richard Zhu
@ 2021-09-08 15:12 ` Bjorn Helgaas
-1 siblings, 0 replies; 25+ messages in thread
From: Bjorn Helgaas @ 2021-09-08 15:12 UTC (permalink / raw)
To: Richard Zhu
Cc: l.stach, bhelgaas, lorenzo.pieralisi, linux-pci, linux-imx,
linux-arm-kernel, linux-kernel, kernel
On Wed, Sep 08, 2021 at 02:59:24PM +0800, Richard Zhu wrote:
> No function changes, just encapsulate the i.MX PCIe clocks enable
> operations into one standalone function
When you update this,
- it's helpful if you include a cover letter with a multi-patch
series, with the patches being replies to the cover letter, and
- please follow the sentence and formatting conventions for subject
lines and commit logs (driver name should match, capitalize
subject line, end sentences with periods, blank lines between
paragraphs, remove useless information like timestamps from log
messages, indent quoted material like logs by two spaces, etc).
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 82 +++++++++++++++++----------
> 1 file changed, 51 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 80fc98acf097..0264432e4c4a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> +
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> @@ -498,33 +500,12 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> }
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_phy clock\n");
> - goto err_pcie_phy;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + ret = imx6_pcie_clk_enable(imx6_pcie);
> if (ret) {
> - dev_err(dev, "unable to enable pcie_bus clock\n");
> - goto err_pcie_bus;
> + dev_err(dev, "unable to enable pcie clocks\n");
> + goto err_clks;
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie clock\n");
> - goto err_pcie;
> - }
> -
> - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie ref clock\n");
> - goto err_ref_clk;
> - }
> -
> - /* allow the clocks to stabilize */
> - usleep_range(200, 500);
> -
> /* Some boards don't have PCIe reset GPIO. */
> if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> @@ -578,13 +559,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)
> @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> usleep_range(1000, 10000);
> }
>
> +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;
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_phy clock\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_bus clock\n");
> + goto err_pcie_bus;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie clock\n");
> + goto err_pcie;
> + }
> +
> + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie ref clock\n");
> + goto err_ref_clk;
> + }
> +
> + /* 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 imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> {
> clk_disable_unprepare(imx6_pcie->pcie);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
@ 2021-09-08 15:12 ` Bjorn Helgaas
0 siblings, 0 replies; 25+ messages in thread
From: Bjorn Helgaas @ 2021-09-08 15:12 UTC (permalink / raw)
To: Richard Zhu
Cc: l.stach, bhelgaas, lorenzo.pieralisi, linux-pci, linux-imx,
linux-arm-kernel, linux-kernel, kernel
On Wed, Sep 08, 2021 at 02:59:24PM +0800, Richard Zhu wrote:
> No function changes, just encapsulate the i.MX PCIe clocks enable
> operations into one standalone function
When you update this,
- it's helpful if you include a cover letter with a multi-patch
series, with the patches being replies to the cover letter, and
- please follow the sentence and formatting conventions for subject
lines and commit logs (driver name should match, capitalize
subject line, end sentences with periods, blank lines between
paragraphs, remove useless information like timestamps from log
messages, indent quoted material like logs by two spaces, etc).
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 82 +++++++++++++++++----------
> 1 file changed, 51 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 80fc98acf097..0264432e4c4a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> +
> static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val)
> {
> struct dw_pcie *pci = imx6_pcie->pci;
> @@ -498,33 +500,12 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> }
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie_phy clock\n");
> - goto err_pcie_phy;
> - }
> -
> - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + ret = imx6_pcie_clk_enable(imx6_pcie);
> if (ret) {
> - dev_err(dev, "unable to enable pcie_bus clock\n");
> - goto err_pcie_bus;
> + dev_err(dev, "unable to enable pcie clocks\n");
> + goto err_clks;
> }
>
> - ret = clk_prepare_enable(imx6_pcie->pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie clock\n");
> - goto err_pcie;
> - }
> -
> - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> - if (ret) {
> - dev_err(dev, "unable to enable pcie ref clock\n");
> - goto err_ref_clk;
> - }
> -
> - /* allow the clocks to stabilize */
> - usleep_range(200, 500);
> -
> /* Some boards don't have PCIe reset GPIO. */
> if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> @@ -578,13 +559,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)
> @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)
> usleep_range(1000, 10000);
> }
>
> +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;
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_phy clock\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie_bus clock\n");
> + goto err_pcie_bus;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie clock\n");
> + goto err_pcie;
> + }
> +
> + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> + if (ret) {
> + dev_err(dev, "unable to enable pcie ref clock\n");
> + goto err_ref_clk;
> + }
> +
> + /* 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 imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> {
> clk_disable_unprepare(imx6_pcie->pcie);
> --
> 2.25.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
2021-09-08 15:12 ` Bjorn Helgaas
@ 2021-09-09 2:26 ` Richard Zhu
-1 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-09 2:26 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: l.stach, bhelgaas, lorenzo.pieralisi, linux-pci, dl-linux-imx,
linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, September 8, 2021 11:12 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one
> standalone function
>
> On Wed, Sep 08, 2021 at 02:59:24PM +0800, Richard Zhu wrote:
> > No function changes, just encapsulate the i.MX PCIe clocks enable
> > operations into one standalone function
>
> When you update this,
>
> - it's helpful if you include a cover letter with a multi-patch
> series, with the patches being replies to the cover letter, and
>
> - please follow the sentence and formatting conventions for subject
> lines and commit logs (driver name should match, capitalize
> subject line, end sentences with periods, blank lines between
> paragraphs, remove useless information like timestamps from log
> messages, indent quoted material like logs by two spaces, etc).
>
[Richard Zhu] Ok, got that. Thanks for your kindly reminder.
Would use the cover letter, and reformat the subject lines and commit logs later.
Best Regards
Richard Zhu
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 82
> > +++++++++++++++++----------
> > 1 file changed, 51 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 80fc98acf097..0264432e4c4a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> > +
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val) {
> > struct dw_pcie *pci = imx6_pcie->pci; @@ -498,33 +500,12 @@ static
> > void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > }
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_phy clock\n");
> > - goto err_pcie_phy;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + ret = imx6_pcie_clk_enable(imx6_pcie);
> > if (ret) {
> > - dev_err(dev, "unable to enable pcie_bus clock\n");
> > - goto err_pcie_bus;
> > + dev_err(dev, "unable to enable pcie clocks\n");
> > + goto err_clks;
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie clock\n");
> > - goto err_pcie;
> > - }
> > -
> > - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie ref clock\n");
> > - goto err_ref_clk;
> > - }
> > -
> > - /* allow the clocks to stabilize */
> > - usleep_range(200, 500);
> > -
> > /* Some boards don't have PCIe reset GPIO. */
> > if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > @@ -578,13 +559,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)
> > @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> > usleep_range(1000, 10000);
> > }
> >
> > +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;
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_phy clock\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_bus clock\n");
> > + goto err_pcie_bus;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie clock\n");
> > + goto err_pcie;
> > + }
> > +
> > + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie ref clock\n");
> > + goto err_ref_clk;
> > + }
> > +
> > + /* 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 imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) {
> > clk_disable_unprepare(imx6_pcie->pcie);
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one standalone function
@ 2021-09-09 2:26 ` Richard Zhu
0 siblings, 0 replies; 25+ messages in thread
From: Richard Zhu @ 2021-09-09 2:26 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: l.stach, bhelgaas, lorenzo.pieralisi, linux-pci, dl-linux-imx,
linux-arm-kernel, linux-kernel, kernel
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, September 8, 2021 11:12 PM
> To: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: l.stach@pengutronix.de; bhelgaas@google.com;
> lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; kernel@pengutronix.de
> Subject: Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one
> standalone function
>
> On Wed, Sep 08, 2021 at 02:59:24PM +0800, Richard Zhu wrote:
> > No function changes, just encapsulate the i.MX PCIe clocks enable
> > operations into one standalone function
>
> When you update this,
>
> - it's helpful if you include a cover letter with a multi-patch
> series, with the patches being replies to the cover letter, and
>
> - please follow the sentence and formatting conventions for subject
> lines and commit logs (driver name should match, capitalize
> subject line, end sentences with periods, blank lines between
> paragraphs, remove useless information like timestamps from log
> messages, indent quoted material like logs by two spaces, etc).
>
[Richard Zhu] Ok, got that. Thanks for your kindly reminder.
Would use the cover letter, and reformat the subject lines and commit logs later.
Best Regards
Richard Zhu
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > drivers/pci/controller/dwc/pci-imx6.c | 82
> > +++++++++++++++++----------
> > 1 file changed, 51 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 80fc98acf097..0264432e4c4a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -143,6 +143,8 @@ 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 int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie);
> > +
> > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
> > exp_val) {
> > struct dw_pcie *pci = imx6_pcie->pci; @@ -498,33 +500,12 @@ static
> > void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > }
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie_phy clock\n");
> > - goto err_pcie_phy;
> > - }
> > -
> > - ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + ret = imx6_pcie_clk_enable(imx6_pcie);
> > if (ret) {
> > - dev_err(dev, "unable to enable pcie_bus clock\n");
> > - goto err_pcie_bus;
> > + dev_err(dev, "unable to enable pcie clocks\n");
> > + goto err_clks;
> > }
> >
> > - ret = clk_prepare_enable(imx6_pcie->pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie clock\n");
> > - goto err_pcie;
> > - }
> > -
> > - ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > - if (ret) {
> > - dev_err(dev, "unable to enable pcie ref clock\n");
> > - goto err_ref_clk;
> > - }
> > -
> > - /* allow the clocks to stabilize */
> > - usleep_range(200, 500);
> > -
> > /* Some boards don't have PCIe reset GPIO. */
> > if (gpio_is_valid(imx6_pcie->reset_gpio)) {
> > gpio_set_value_cansleep(imx6_pcie->reset_gpio,
> > @@ -578,13 +559,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)
> > @@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct
> imx6_pcie *imx6_pcie)
> > usleep_range(1000, 10000);
> > }
> >
> > +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;
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_phy);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_phy clock\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie_bus);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie_bus clock\n");
> > + goto err_pcie_bus;
> > + }
> > +
> > + ret = clk_prepare_enable(imx6_pcie->pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie clock\n");
> > + goto err_pcie;
> > + }
> > +
> > + ret = imx6_pcie_enable_ref_clk(imx6_pcie);
> > + if (ret) {
> > + dev_err(dev, "unable to enable pcie ref clock\n");
> > + goto err_ref_clk;
> > + }
> > +
> > + /* 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 imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) {
> > clk_disable_unprepare(imx6_pcie->pcie);
> > --
> > 2.25.1
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 25+ messages in thread