* [PATCH v4 1/2] PCI: qcom: Move IPQ8074 DBI register accesses after phy_power_on()
@ 2022-06-24 10:44 Robert Marko
2022-06-24 10:44 ` [PATCH v4 2/2] PCI: qcom: Move all " Robert Marko
2022-06-29 9:21 ` [PATCH v4 1/2] PCI: qcom: Move IPQ8074 " Robert Marko
0 siblings, 2 replies; 11+ messages in thread
From: Robert Marko @ 2022-06-24 10:44 UTC (permalink / raw)
To: svarbanov, agross, bjorn.andersson, lpieralisi, robh, kw,
bhelgaas, p.zabel, jingoohan1, linux-pci, linux-arm-msm,
linux-kernel, johan+linaro, dmitry.baryshkov
Cc: Robert Marko
Currently the Gen2 port in IPQ8074 will cause the system to hang as it
accesses DBI registers in qcom_pcie_init_2_3_3(), and those are only
accesible after phy_power_on().
Move the DBI read/writes to a new qcom_pcie_post_init_2_3_3(), which is
executed after phy_power_on().
Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code")
Signed-off-by: Robert Marko <robimarko@gmail.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Changes in v4:
* Correct title and description
Changes in v3:
* Make sure it applies onto 5.19-rc3
* Update the commit description to make it clear this only affects the
Gen2 port
Changes in v2:
* Rebase onto next-20220621
---
drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++-----------
1 file changed, 28 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index a1f1aca2fb59..24708d5d817d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1061,9 +1061,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
struct qcom_pcie_resources_2_3_3 *res = &pcie->res.v2_3_3;
struct dw_pcie *pci = pcie->pci;
struct device *dev = pci->dev;
- u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
int i, ret;
- u32 val;
for (i = 0; i < ARRAY_SIZE(res->rst); i++) {
ret = reset_control_assert(res->rst[i]);
@@ -1120,6 +1118,33 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
goto err_clk_aux;
}
+ return 0;
+
+err_clk_aux:
+ clk_disable_unprepare(res->ahb_clk);
+err_clk_ahb:
+ clk_disable_unprepare(res->axi_s_clk);
+err_clk_axi_s:
+ clk_disable_unprepare(res->axi_m_clk);
+err_clk_axi_m:
+ clk_disable_unprepare(res->iface);
+err_clk_iface:
+ /*
+ * Not checking for failure, will anyway return
+ * the original failure in 'ret'.
+ */
+ for (i = 0; i < ARRAY_SIZE(res->rst); i++)
+ reset_control_assert(res->rst[i]);
+
+ return ret;
+}
+
+static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
+{
+ struct dw_pcie *pci = pcie->pci;
+ u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+ u32 val;
+
writel(SLV_ADDR_SPACE_SZ,
pcie->parf + PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE);
@@ -1147,24 +1172,6 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
PCI_EXP_DEVCTL2);
return 0;
-
-err_clk_aux:
- clk_disable_unprepare(res->ahb_clk);
-err_clk_ahb:
- clk_disable_unprepare(res->axi_s_clk);
-err_clk_axi_s:
- clk_disable_unprepare(res->axi_m_clk);
-err_clk_axi_m:
- clk_disable_unprepare(res->iface);
-err_clk_iface:
- /*
- * Not checking for failure, will anyway return
- * the original failure in 'ret'.
- */
- for (i = 0; i < ARRAY_SIZE(res->rst); i++)
- reset_control_assert(res->rst[i]);
-
- return ret;
}
static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
@@ -1596,6 +1603,7 @@ static const struct qcom_pcie_ops ops_2_4_0 = {
static const struct qcom_pcie_ops ops_2_3_3 = {
.get_resources = qcom_pcie_get_resources_2_3_3,
.init = qcom_pcie_init_2_3_3,
+ .post_init = qcom_pcie_post_init_2_3_3,
.deinit = qcom_pcie_deinit_2_3_3,
.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
};
--
2.36.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-06-24 10:44 [PATCH v4 1/2] PCI: qcom: Move IPQ8074 DBI register accesses after phy_power_on() Robert Marko
@ 2022-06-24 10:44 ` Robert Marko
2022-07-07 19:41 ` Bjorn Helgaas
2022-06-29 9:21 ` [PATCH v4 1/2] PCI: qcom: Move IPQ8074 " Robert Marko
1 sibling, 1 reply; 11+ messages in thread
From: Robert Marko @ 2022-06-24 10:44 UTC (permalink / raw)
To: svarbanov, agross, bjorn.andersson, lpieralisi, robh, kw,
bhelgaas, p.zabel, jingoohan1, linux-pci, linux-arm-msm,
linux-kernel, johan+linaro, dmitry.baryshkov
Cc: Robert Marko
IPQ8074 requires the PHY to be powered on before accessing DBI registers.
It's not clear whether other variants have the same dependency, but there
seems to be no reason for them to be different, so move all the DBI
accesses from .init() to .post_init() so they are all after phy_power_on().
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
Changes in v4:
* Move 2.7.0 accesses as well
* Correct title and description (Bjorn)
---
drivers/pci/controller/dwc/pcie-qcom.c | 215 ++++++++++++++-----------
1 file changed, 119 insertions(+), 96 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 24708d5d817d..f1a156052fe7 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -348,8 +348,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
struct dw_pcie *pci = pcie->pci;
struct device *dev = pci->dev;
- struct device_node *node = dev->of_node;
- u32 val;
int ret;
/* reset the PCIe interface as uboot can leave it undefined state */
@@ -360,8 +358,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
reset_control_assert(res->ext_reset);
reset_control_assert(res->phy_reset);
- writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
-
ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
if (ret < 0) {
dev_err(dev, "cannot enable regulators\n");
@@ -408,6 +404,35 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
if (ret)
goto err_clks;
+ return 0;
+
+err_clks:
+ reset_control_assert(res->axi_reset);
+err_deassert_axi:
+ reset_control_assert(res->por_reset);
+err_deassert_por:
+ reset_control_assert(res->pci_reset);
+err_deassert_pci:
+ reset_control_assert(res->phy_reset);
+err_deassert_phy:
+ reset_control_assert(res->ext_reset);
+err_deassert_ext:
+ reset_control_assert(res->ahb_reset);
+err_deassert_ahb:
+ regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
+
+ return ret;
+}
+
+static int qcom_pcie_post_init_2_1_0(struct qcom_pcie *pcie)
+{
+ struct dw_pcie *pci = pcie->pci;
+ struct device *dev = pci->dev;
+ struct device_node *node = dev->of_node;
+ u32 val;
+
+ writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
/* enable PCIe clocks and resets */
val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
val &= ~BIT(0);
@@ -451,23 +476,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
return 0;
-
-err_clks:
- reset_control_assert(res->axi_reset);
-err_deassert_axi:
- reset_control_assert(res->por_reset);
-err_deassert_por:
- reset_control_assert(res->pci_reset);
-err_deassert_pci:
- reset_control_assert(res->phy_reset);
-err_deassert_phy:
- reset_control_assert(res->ext_reset);
-err_deassert_ext:
- reset_control_assert(res->ahb_reset);
-err_deassert_ahb:
- regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
-
- return ret;
}
static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
@@ -555,16 +563,6 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
goto err_slave;
}
- /* change DBI base address */
- writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
-
- if (IS_ENABLED(CONFIG_PCI_MSI)) {
- u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
-
- val |= BIT(31);
- writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
- }
-
return 0;
err_slave:
clk_disable_unprepare(res->slave_bus);
@@ -580,6 +578,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
return ret;
}
+static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
+{
+
+ /* change DBI base address */
+ writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+
+ val |= BIT(31);
+ writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+ }
+
+ return 0;
+}
+
static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
{
u32 val;
@@ -648,7 +662,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
struct dw_pcie *pci = pcie->pci;
struct device *dev = pci->dev;
- u32 val;
int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -681,27 +694,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
goto err_slave_clk;
}
- /* enable PCIe clocks and resets */
- val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
- val &= ~BIT(0);
- writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
-
- /* change DBI base address */
- writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
-
- /* MAC PHY_POWERDOWN MUX DISABLE */
- val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
- val &= ~BIT(29);
- writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
-
- val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
- val |= BIT(4);
- writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
-
- val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
- val |= BIT(31);
- writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
-
return 0;
err_slave_clk:
@@ -722,8 +714,30 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
struct dw_pcie *pci = pcie->pci;
struct device *dev = pci->dev;
+ u32 val;
int ret;
+ /* enable PCIe clocks and resets */
+ val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
+ val &= ~BIT(0);
+ writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
+ /* change DBI base address */
+ writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+ /* MAC PHY_POWERDOWN MUX DISABLE */
+ val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
+ val &= ~BIT(29);
+ writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
+
+ val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+ val |= BIT(4);
+ writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+
+ val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
+ val |= BIT(31);
+ writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
+
ret = clk_prepare_enable(res->pipe_clk);
if (ret) {
dev_err(dev, "cannot prepare/enable pipe clock\n");
@@ -837,7 +851,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
struct dw_pcie *pci = pcie->pci;
struct device *dev = pci->dev;
- u32 val;
int ret;
ret = reset_control_assert(res->axi_m_reset);
@@ -962,6 +975,33 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
if (ret)
goto err_clks;
+ return 0;
+
+err_clks:
+ reset_control_assert(res->ahb_reset);
+err_rst_ahb:
+ reset_control_assert(res->pwr_reset);
+err_rst_pwr:
+ reset_control_assert(res->axi_s_reset);
+err_rst_axi_s:
+ reset_control_assert(res->axi_m_sticky_reset);
+err_rst_axi_m_sticky:
+ reset_control_assert(res->axi_m_reset);
+err_rst_axi_m:
+ reset_control_assert(res->pipe_sticky_reset);
+err_rst_pipe_sticky:
+ reset_control_assert(res->pipe_reset);
+err_rst_pipe:
+ reset_control_assert(res->phy_reset);
+err_rst_phy:
+ reset_control_assert(res->phy_ahb_reset);
+ return ret;
+}
+
+static int qcom_pcie_post_init_2_4_0(struct qcom_pcie *pcie)
+{
+ u32 val;
+
/* enable PCIe clocks and resets */
val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
val &= ~BIT(0);
@@ -984,26 +1024,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
return 0;
-
-err_clks:
- reset_control_assert(res->ahb_reset);
-err_rst_ahb:
- reset_control_assert(res->pwr_reset);
-err_rst_pwr:
- reset_control_assert(res->axi_s_reset);
-err_rst_axi_s:
- reset_control_assert(res->axi_m_sticky_reset);
-err_rst_axi_m_sticky:
- reset_control_assert(res->axi_m_reset);
-err_rst_axi_m:
- reset_control_assert(res->pipe_sticky_reset);
-err_rst_pipe_sticky:
- reset_control_assert(res->pipe_reset);
-err_rst_pipe:
- reset_control_assert(res->phy_reset);
-err_rst_phy:
- reset_control_assert(res->phy_ahb_reset);
- return ret;
}
static int qcom_pcie_get_resources_2_3_3(struct qcom_pcie *pcie)
@@ -1237,7 +1257,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
struct dw_pcie *pci = pcie->pci;
struct device *dev = pci->dev;
- u32 val;
int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
@@ -1271,6 +1290,28 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
/* Wait for reset to complete, required on SM8450 */
usleep_range(1000, 1500);
+ return 0;
+err_disable_clocks:
+ clk_bulk_disable_unprepare(res->num_clks, res->clks);
+err_disable_regulators:
+ regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
+
+ return ret;
+}
+
+static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+
+ clk_bulk_disable_unprepare(res->num_clks, res->clks);
+ regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
+}
+
+static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
+ u32 val;
+
/* configure PCIe to RC mode */
writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
@@ -1297,27 +1338,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
}
- return 0;
-err_disable_clocks:
- clk_bulk_disable_unprepare(res->num_clks, res->clks);
-err_disable_regulators:
- regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
-
- return ret;
-}
-
-static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
-{
- struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
-
- clk_bulk_disable_unprepare(res->num_clks, res->clks);
- regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
-}
-
-static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
-{
- struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
-
/* Set pipe clock as clock source for pcie_pipe_clk_src */
if (pcie->cfg->pipe_clk_need_muxing)
clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
@@ -1569,6 +1589,7 @@ static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
static const struct qcom_pcie_ops ops_2_1_0 = {
.get_resources = qcom_pcie_get_resources_2_1_0,
.init = qcom_pcie_init_2_1_0,
+ .post_init = qcom_pcie_post_init_2_1_0,
.deinit = qcom_pcie_deinit_2_1_0,
.ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
};
@@ -1577,6 +1598,7 @@ static const struct qcom_pcie_ops ops_2_1_0 = {
static const struct qcom_pcie_ops ops_1_0_0 = {
.get_resources = qcom_pcie_get_resources_1_0_0,
.init = qcom_pcie_init_1_0_0,
+ .post_init = qcom_pcie_post_init_1_0_0,
.deinit = qcom_pcie_deinit_1_0_0,
.ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
};
@@ -1595,6 +1617,7 @@ static const struct qcom_pcie_ops ops_2_3_2 = {
static const struct qcom_pcie_ops ops_2_4_0 = {
.get_resources = qcom_pcie_get_resources_2_4_0,
.init = qcom_pcie_init_2_4_0,
+ .post_init = qcom_pcie_post_init_2_4_0,
.deinit = qcom_pcie_deinit_2_4_0,
.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
};
--
2.36.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] PCI: qcom: Move IPQ8074 DBI register accesses after phy_power_on()
2022-06-24 10:44 [PATCH v4 1/2] PCI: qcom: Move IPQ8074 DBI register accesses after phy_power_on() Robert Marko
2022-06-24 10:44 ` [PATCH v4 2/2] PCI: qcom: Move all " Robert Marko
@ 2022-06-29 9:21 ` Robert Marko
1 sibling, 0 replies; 11+ messages in thread
From: Robert Marko @ 2022-06-29 9:21 UTC (permalink / raw)
To: Stanimir Varbanov, Andy Gross, Bjorn Andersson, lpieralisi,
Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1, linux-pci,
linux-arm-msm, open list, johan+linaro, Dmitry Baryshkov
On Fri, 24 Jun 2022 at 12:44, Robert Marko <robimarko@gmail.com> wrote:
>
> Currently the Gen2 port in IPQ8074 will cause the system to hang as it
> accesses DBI registers in qcom_pcie_init_2_3_3(), and those are only
> accesible after phy_power_on().
>
> Move the DBI read/writes to a new qcom_pcie_post_init_2_3_3(), which is
> executed after phy_power_on().
>
> Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code")
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Hi,
Bjorn, is there something else I need to fixup?
Regards,
Robert
> ---
> Changes in v4:
> * Correct title and description
>
> Changes in v3:
> * Make sure it applies onto 5.19-rc3
> * Update the commit description to make it clear this only affects the
> Gen2 port
>
> Changes in v2:
> * Rebase onto next-20220621
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++-----------
> 1 file changed, 28 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index a1f1aca2fb59..24708d5d817d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1061,9 +1061,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
> struct qcom_pcie_resources_2_3_3 *res = &pcie->res.v2_3_3;
> struct dw_pcie *pci = pcie->pci;
> struct device *dev = pci->dev;
> - u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> int i, ret;
> - u32 val;
>
> for (i = 0; i < ARRAY_SIZE(res->rst); i++) {
> ret = reset_control_assert(res->rst[i]);
> @@ -1120,6 +1118,33 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
> goto err_clk_aux;
> }
>
> + return 0;
> +
> +err_clk_aux:
> + clk_disable_unprepare(res->ahb_clk);
> +err_clk_ahb:
> + clk_disable_unprepare(res->axi_s_clk);
> +err_clk_axi_s:
> + clk_disable_unprepare(res->axi_m_clk);
> +err_clk_axi_m:
> + clk_disable_unprepare(res->iface);
> +err_clk_iface:
> + /*
> + * Not checking for failure, will anyway return
> + * the original failure in 'ret'.
> + */
> + for (i = 0; i < ARRAY_SIZE(res->rst); i++)
> + reset_control_assert(res->rst[i]);
> +
> + return ret;
> +}
> +
> +static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
> +{
> + struct dw_pcie *pci = pcie->pci;
> + u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> + u32 val;
> +
> writel(SLV_ADDR_SPACE_SZ,
> pcie->parf + PCIE20_v3_PARF_SLV_ADDR_SPACE_SIZE);
>
> @@ -1147,24 +1172,6 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
> PCI_EXP_DEVCTL2);
>
> return 0;
> -
> -err_clk_aux:
> - clk_disable_unprepare(res->ahb_clk);
> -err_clk_ahb:
> - clk_disable_unprepare(res->axi_s_clk);
> -err_clk_axi_s:
> - clk_disable_unprepare(res->axi_m_clk);
> -err_clk_axi_m:
> - clk_disable_unprepare(res->iface);
> -err_clk_iface:
> - /*
> - * Not checking for failure, will anyway return
> - * the original failure in 'ret'.
> - */
> - for (i = 0; i < ARRAY_SIZE(res->rst); i++)
> - reset_control_assert(res->rst[i]);
> -
> - return ret;
> }
>
> static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
> @@ -1596,6 +1603,7 @@ static const struct qcom_pcie_ops ops_2_4_0 = {
> static const struct qcom_pcie_ops ops_2_3_3 = {
> .get_resources = qcom_pcie_get_resources_2_3_3,
> .init = qcom_pcie_init_2_3_3,
> + .post_init = qcom_pcie_post_init_2_3_3,
> .deinit = qcom_pcie_deinit_2_3_3,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> };
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-06-24 10:44 ` [PATCH v4 2/2] PCI: qcom: Move all " Robert Marko
@ 2022-07-07 19:41 ` Bjorn Helgaas
2022-07-08 16:39 ` Robert Marko
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2022-07-07 19:41 UTC (permalink / raw)
To: Robert Marko
Cc: svarbanov, agross, bjorn.andersson, lpieralisi, robh, kw,
bhelgaas, p.zabel, jingoohan1, linux-pci, linux-arm-msm,
linux-kernel, johan+linaro, dmitry.baryshkov
On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> It's not clear whether other variants have the same dependency, but there
> seems to be no reason for them to be different, so move all the DBI
> accesses from .init() to .post_init() so they are all after phy_power_on().
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
Would any of the qcom driver folks care to review and ack this?
Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
> ---
> Changes in v4:
> * Move 2.7.0 accesses as well
> * Correct title and description (Bjorn)
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 215 ++++++++++++++-----------
> 1 file changed, 119 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 24708d5d817d..f1a156052fe7 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -348,8 +348,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
> struct dw_pcie *pci = pcie->pci;
> struct device *dev = pci->dev;
> - struct device_node *node = dev->of_node;
> - u32 val;
> int ret;
>
> /* reset the PCIe interface as uboot can leave it undefined state */
> @@ -360,8 +358,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> reset_control_assert(res->ext_reset);
> reset_control_assert(res->phy_reset);
>
> - writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> -
> ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> if (ret < 0) {
> dev_err(dev, "cannot enable regulators\n");
> @@ -408,6 +404,35 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> if (ret)
> goto err_clks;
>
> + return 0;
> +
> +err_clks:
> + reset_control_assert(res->axi_reset);
> +err_deassert_axi:
> + reset_control_assert(res->por_reset);
> +err_deassert_por:
> + reset_control_assert(res->pci_reset);
> +err_deassert_pci:
> + reset_control_assert(res->phy_reset);
> +err_deassert_phy:
> + reset_control_assert(res->ext_reset);
> +err_deassert_ext:
> + reset_control_assert(res->ahb_reset);
> +err_deassert_ahb:
> + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> +
> + return ret;
> +}
> +
> +static int qcom_pcie_post_init_2_1_0(struct qcom_pcie *pcie)
> +{
> + struct dw_pcie *pci = pcie->pci;
> + struct device *dev = pci->dev;
> + struct device_node *node = dev->of_node;
> + u32 val;
> +
> + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> /* enable PCIe clocks and resets */
> val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> val &= ~BIT(0);
> @@ -451,23 +476,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
>
> return 0;
> -
> -err_clks:
> - reset_control_assert(res->axi_reset);
> -err_deassert_axi:
> - reset_control_assert(res->por_reset);
> -err_deassert_por:
> - reset_control_assert(res->pci_reset);
> -err_deassert_pci:
> - reset_control_assert(res->phy_reset);
> -err_deassert_phy:
> - reset_control_assert(res->ext_reset);
> -err_deassert_ext:
> - reset_control_assert(res->ahb_reset);
> -err_deassert_ahb:
> - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> -
> - return ret;
> }
>
> static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
> @@ -555,16 +563,6 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> goto err_slave;
> }
>
> - /* change DBI base address */
> - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> -
> - if (IS_ENABLED(CONFIG_PCI_MSI)) {
> - u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> -
> - val |= BIT(31);
> - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> - }
> -
> return 0;
> err_slave:
> clk_disable_unprepare(res->slave_bus);
> @@ -580,6 +578,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> return ret;
> }
>
> +static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> +{
> +
> + /* change DBI base address */
> + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> + u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> +
> + val |= BIT(31);
> + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> + }
> +
> + return 0;
> +}
> +
> static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
> {
> u32 val;
> @@ -648,7 +662,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> struct dw_pcie *pci = pcie->pci;
> struct device *dev = pci->dev;
> - u32 val;
> int ret;
>
> ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> @@ -681,27 +694,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> goto err_slave_clk;
> }
>
> - /* enable PCIe clocks and resets */
> - val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> - val &= ~BIT(0);
> - writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> -
> - /* change DBI base address */
> - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> -
> - /* MAC PHY_POWERDOWN MUX DISABLE */
> - val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> - val &= ~BIT(29);
> - writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> -
> - val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> - val |= BIT(4);
> - writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> -
> - val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> - val |= BIT(31);
> - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> -
> return 0;
>
> err_slave_clk:
> @@ -722,8 +714,30 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
> struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> struct dw_pcie *pci = pcie->pci;
> struct device *dev = pci->dev;
> + u32 val;
> int ret;
>
> + /* enable PCIe clocks and resets */
> + val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> + val &= ~BIT(0);
> + writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> + /* change DBI base address */
> + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> + /* MAC PHY_POWERDOWN MUX DISABLE */
> + val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> + val &= ~BIT(29);
> + writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> +
> + val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> + val |= BIT(4);
> + writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +
> + val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> + val |= BIT(31);
> + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> +
> ret = clk_prepare_enable(res->pipe_clk);
> if (ret) {
> dev_err(dev, "cannot prepare/enable pipe clock\n");
> @@ -837,7 +851,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> struct dw_pcie *pci = pcie->pci;
> struct device *dev = pci->dev;
> - u32 val;
> int ret;
>
> ret = reset_control_assert(res->axi_m_reset);
> @@ -962,6 +975,33 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> if (ret)
> goto err_clks;
>
> + return 0;
> +
> +err_clks:
> + reset_control_assert(res->ahb_reset);
> +err_rst_ahb:
> + reset_control_assert(res->pwr_reset);
> +err_rst_pwr:
> + reset_control_assert(res->axi_s_reset);
> +err_rst_axi_s:
> + reset_control_assert(res->axi_m_sticky_reset);
> +err_rst_axi_m_sticky:
> + reset_control_assert(res->axi_m_reset);
> +err_rst_axi_m:
> + reset_control_assert(res->pipe_sticky_reset);
> +err_rst_pipe_sticky:
> + reset_control_assert(res->pipe_reset);
> +err_rst_pipe:
> + reset_control_assert(res->phy_reset);
> +err_rst_phy:
> + reset_control_assert(res->phy_ahb_reset);
> + return ret;
> +}
> +
> +static int qcom_pcie_post_init_2_4_0(struct qcom_pcie *pcie)
> +{
> + u32 val;
> +
> /* enable PCIe clocks and resets */
> val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> val &= ~BIT(0);
> @@ -984,26 +1024,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
>
> return 0;
> -
> -err_clks:
> - reset_control_assert(res->ahb_reset);
> -err_rst_ahb:
> - reset_control_assert(res->pwr_reset);
> -err_rst_pwr:
> - reset_control_assert(res->axi_s_reset);
> -err_rst_axi_s:
> - reset_control_assert(res->axi_m_sticky_reset);
> -err_rst_axi_m_sticky:
> - reset_control_assert(res->axi_m_reset);
> -err_rst_axi_m:
> - reset_control_assert(res->pipe_sticky_reset);
> -err_rst_pipe_sticky:
> - reset_control_assert(res->pipe_reset);
> -err_rst_pipe:
> - reset_control_assert(res->phy_reset);
> -err_rst_phy:
> - reset_control_assert(res->phy_ahb_reset);
> - return ret;
> }
>
> static int qcom_pcie_get_resources_2_3_3(struct qcom_pcie *pcie)
> @@ -1237,7 +1257,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> struct dw_pcie *pci = pcie->pci;
> struct device *dev = pci->dev;
> - u32 val;
> int ret;
>
> ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> @@ -1271,6 +1290,28 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> /* Wait for reset to complete, required on SM8450 */
> usleep_range(1000, 1500);
>
> + return 0;
> +err_disable_clocks:
> + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> +err_disable_regulators:
> + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> +
> + return ret;
> +}
> +
> +static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> +{
> + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> +
> + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> +}
> +
> +static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> +{
> + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> + u32 val;
> +
> /* configure PCIe to RC mode */
> writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
>
> @@ -1297,27 +1338,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> }
>
> - return 0;
> -err_disable_clocks:
> - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> -err_disable_regulators:
> - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> -
> - return ret;
> -}
> -
> -static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> -{
> - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> -
> - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> -}
> -
> -static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> -{
> - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> -
> /* Set pipe clock as clock source for pcie_pipe_clk_src */
> if (pcie->cfg->pipe_clk_need_muxing)
> clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
> @@ -1569,6 +1589,7 @@ static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> static const struct qcom_pcie_ops ops_2_1_0 = {
> .get_resources = qcom_pcie_get_resources_2_1_0,
> .init = qcom_pcie_init_2_1_0,
> + .post_init = qcom_pcie_post_init_2_1_0,
> .deinit = qcom_pcie_deinit_2_1_0,
> .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> };
> @@ -1577,6 +1598,7 @@ static const struct qcom_pcie_ops ops_2_1_0 = {
> static const struct qcom_pcie_ops ops_1_0_0 = {
> .get_resources = qcom_pcie_get_resources_1_0_0,
> .init = qcom_pcie_init_1_0_0,
> + .post_init = qcom_pcie_post_init_1_0_0,
> .deinit = qcom_pcie_deinit_1_0_0,
> .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> };
> @@ -1595,6 +1617,7 @@ static const struct qcom_pcie_ops ops_2_3_2 = {
> static const struct qcom_pcie_ops ops_2_4_0 = {
> .get_resources = qcom_pcie_get_resources_2_4_0,
> .init = qcom_pcie_init_2_4_0,
> + .post_init = qcom_pcie_post_init_2_4_0,
> .deinit = qcom_pcie_deinit_2_4_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> };
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-07-07 19:41 ` Bjorn Helgaas
@ 2022-07-08 16:39 ` Robert Marko
2022-07-08 16:47 ` Christian Marangi
0 siblings, 1 reply; 11+ messages in thread
From: Robert Marko @ 2022-07-08 16:39 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Stanimir Varbanov, Andy Gross, Bjorn Andersson, lpieralisi,
Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1, linux-pci,
linux-arm-msm, open list, johan+linaro, Dmitry Baryshkov,
Ansuel Smith
CC-ing Christian who did a lot of work on 2.1.0 (IPQ806x).
Regards,
Robert
On Thu, 7 Jul 2022 at 21:41, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> > IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> > It's not clear whether other variants have the same dependency, but there
> > seems to be no reason for them to be different, so move all the DBI
> > accesses from .init() to .post_init() so they are all after phy_power_on().
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
>
> Would any of the qcom driver folks care to review and ack this?
> Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
>
> > ---
> > Changes in v4:
> > * Move 2.7.0 accesses as well
> > * Correct title and description (Bjorn)
> > ---
> > drivers/pci/controller/dwc/pcie-qcom.c | 215 ++++++++++++++-----------
> > 1 file changed, 119 insertions(+), 96 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 24708d5d817d..f1a156052fe7 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -348,8 +348,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
> > struct dw_pcie *pci = pcie->pci;
> > struct device *dev = pci->dev;
> > - struct device_node *node = dev->of_node;
> > - u32 val;
> > int ret;
> >
> > /* reset the PCIe interface as uboot can leave it undefined state */
> > @@ -360,8 +358,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > reset_control_assert(res->ext_reset);
> > reset_control_assert(res->phy_reset);
> >
> > - writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > -
> > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > if (ret < 0) {
> > dev_err(dev, "cannot enable regulators\n");
> > @@ -408,6 +404,35 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > if (ret)
> > goto err_clks;
> >
> > + return 0;
> > +
> > +err_clks:
> > + reset_control_assert(res->axi_reset);
> > +err_deassert_axi:
> > + reset_control_assert(res->por_reset);
> > +err_deassert_por:
> > + reset_control_assert(res->pci_reset);
> > +err_deassert_pci:
> > + reset_control_assert(res->phy_reset);
> > +err_deassert_phy:
> > + reset_control_assert(res->ext_reset);
> > +err_deassert_ext:
> > + reset_control_assert(res->ahb_reset);
> > +err_deassert_ahb:
> > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > +
> > + return ret;
> > +}
> > +
> > +static int qcom_pcie_post_init_2_1_0(struct qcom_pcie *pcie)
> > +{
> > + struct dw_pcie *pci = pcie->pci;
> > + struct device *dev = pci->dev;
> > + struct device_node *node = dev->of_node;
> > + u32 val;
> > +
> > + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > +
> > /* enable PCIe clocks and resets */
> > val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > val &= ~BIT(0);
> > @@ -451,23 +476,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
> >
> > return 0;
> > -
> > -err_clks:
> > - reset_control_assert(res->axi_reset);
> > -err_deassert_axi:
> > - reset_control_assert(res->por_reset);
> > -err_deassert_por:
> > - reset_control_assert(res->pci_reset);
> > -err_deassert_pci:
> > - reset_control_assert(res->phy_reset);
> > -err_deassert_phy:
> > - reset_control_assert(res->ext_reset);
> > -err_deassert_ext:
> > - reset_control_assert(res->ahb_reset);
> > -err_deassert_ahb:
> > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > -
> > - return ret;
> > }
> >
> > static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
> > @@ -555,16 +563,6 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> > goto err_slave;
> > }
> >
> > - /* change DBI base address */
> > - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > -
> > - if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > - u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > -
> > - val |= BIT(31);
> > - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > - }
> > -
> > return 0;
> > err_slave:
> > clk_disable_unprepare(res->slave_bus);
> > @@ -580,6 +578,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> > return ret;
> > }
> >
> > +static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> > +{
> > +
> > + /* change DBI base address */
> > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > +
> > + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > + u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > +
> > + val |= BIT(31);
> > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
> > {
> > u32 val;
> > @@ -648,7 +662,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> > struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> > struct dw_pcie *pci = pcie->pci;
> > struct device *dev = pci->dev;
> > - u32 val;
> > int ret;
> >
> > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > @@ -681,27 +694,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> > goto err_slave_clk;
> > }
> >
> > - /* enable PCIe clocks and resets */
> > - val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > - val &= ~BIT(0);
> > - writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > -
> > - /* change DBI base address */
> > - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > -
> > - /* MAC PHY_POWERDOWN MUX DISABLE */
> > - val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> > - val &= ~BIT(29);
> > - writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> > -
> > - val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > - val |= BIT(4);
> > - writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > -
> > - val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > - val |= BIT(31);
> > - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > -
> > return 0;
> >
> > err_slave_clk:
> > @@ -722,8 +714,30 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
> > struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> > struct dw_pcie *pci = pcie->pci;
> > struct device *dev = pci->dev;
> > + u32 val;
> > int ret;
> >
> > + /* enable PCIe clocks and resets */
> > + val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > + val &= ~BIT(0);
> > + writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > +
> > + /* change DBI base address */
> > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > +
> > + /* MAC PHY_POWERDOWN MUX DISABLE */
> > + val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> > + val &= ~BIT(29);
> > + writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> > +
> > + val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > + val |= BIT(4);
> > + writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > +
> > + val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > + val |= BIT(31);
> > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > +
> > ret = clk_prepare_enable(res->pipe_clk);
> > if (ret) {
> > dev_err(dev, "cannot prepare/enable pipe clock\n");
> > @@ -837,7 +851,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> > struct dw_pcie *pci = pcie->pci;
> > struct device *dev = pci->dev;
> > - u32 val;
> > int ret;
> >
> > ret = reset_control_assert(res->axi_m_reset);
> > @@ -962,6 +975,33 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > if (ret)
> > goto err_clks;
> >
> > + return 0;
> > +
> > +err_clks:
> > + reset_control_assert(res->ahb_reset);
> > +err_rst_ahb:
> > + reset_control_assert(res->pwr_reset);
> > +err_rst_pwr:
> > + reset_control_assert(res->axi_s_reset);
> > +err_rst_axi_s:
> > + reset_control_assert(res->axi_m_sticky_reset);
> > +err_rst_axi_m_sticky:
> > + reset_control_assert(res->axi_m_reset);
> > +err_rst_axi_m:
> > + reset_control_assert(res->pipe_sticky_reset);
> > +err_rst_pipe_sticky:
> > + reset_control_assert(res->pipe_reset);
> > +err_rst_pipe:
> > + reset_control_assert(res->phy_reset);
> > +err_rst_phy:
> > + reset_control_assert(res->phy_ahb_reset);
> > + return ret;
> > +}
> > +
> > +static int qcom_pcie_post_init_2_4_0(struct qcom_pcie *pcie)
> > +{
> > + u32 val;
> > +
> > /* enable PCIe clocks and resets */
> > val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > val &= ~BIT(0);
> > @@ -984,26 +1024,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> >
> > return 0;
> > -
> > -err_clks:
> > - reset_control_assert(res->ahb_reset);
> > -err_rst_ahb:
> > - reset_control_assert(res->pwr_reset);
> > -err_rst_pwr:
> > - reset_control_assert(res->axi_s_reset);
> > -err_rst_axi_s:
> > - reset_control_assert(res->axi_m_sticky_reset);
> > -err_rst_axi_m_sticky:
> > - reset_control_assert(res->axi_m_reset);
> > -err_rst_axi_m:
> > - reset_control_assert(res->pipe_sticky_reset);
> > -err_rst_pipe_sticky:
> > - reset_control_assert(res->pipe_reset);
> > -err_rst_pipe:
> > - reset_control_assert(res->phy_reset);
> > -err_rst_phy:
> > - reset_control_assert(res->phy_ahb_reset);
> > - return ret;
> > }
> >
> > static int qcom_pcie_get_resources_2_3_3(struct qcom_pcie *pcie)
> > @@ -1237,7 +1257,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > struct dw_pcie *pci = pcie->pci;
> > struct device *dev = pci->dev;
> > - u32 val;
> > int ret;
> >
> > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > @@ -1271,6 +1290,28 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > /* Wait for reset to complete, required on SM8450 */
> > usleep_range(1000, 1500);
> >
> > + return 0;
> > +err_disable_clocks:
> > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > +err_disable_regulators:
> > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > +
> > + return ret;
> > +}
> > +
> > +static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > +{
> > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > +
> > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > +}
> > +
> > +static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> > +{
> > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > + u32 val;
> > +
> > /* configure PCIe to RC mode */
> > writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> >
> > @@ -1297,27 +1338,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > }
> >
> > - return 0;
> > -err_disable_clocks:
> > - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > -err_disable_regulators:
> > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > -
> > - return ret;
> > -}
> > -
> > -static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > -{
> > - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > -
> > - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > -}
> > -
> > -static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> > -{
> > - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > -
> > /* Set pipe clock as clock source for pcie_pipe_clk_src */
> > if (pcie->cfg->pipe_clk_need_muxing)
> > clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
> > @@ -1569,6 +1589,7 @@ static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> > static const struct qcom_pcie_ops ops_2_1_0 = {
> > .get_resources = qcom_pcie_get_resources_2_1_0,
> > .init = qcom_pcie_init_2_1_0,
> > + .post_init = qcom_pcie_post_init_2_1_0,
> > .deinit = qcom_pcie_deinit_2_1_0,
> > .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> > };
> > @@ -1577,6 +1598,7 @@ static const struct qcom_pcie_ops ops_2_1_0 = {
> > static const struct qcom_pcie_ops ops_1_0_0 = {
> > .get_resources = qcom_pcie_get_resources_1_0_0,
> > .init = qcom_pcie_init_1_0_0,
> > + .post_init = qcom_pcie_post_init_1_0_0,
> > .deinit = qcom_pcie_deinit_1_0_0,
> > .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> > };
> > @@ -1595,6 +1617,7 @@ static const struct qcom_pcie_ops ops_2_3_2 = {
> > static const struct qcom_pcie_ops ops_2_4_0 = {
> > .get_resources = qcom_pcie_get_resources_2_4_0,
> > .init = qcom_pcie_init_2_4_0,
> > + .post_init = qcom_pcie_post_init_2_4_0,
> > .deinit = qcom_pcie_deinit_2_4_0,
> > .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > };
> > --
> > 2.36.1
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-07-08 16:39 ` Robert Marko
@ 2022-07-08 16:47 ` Christian Marangi
2022-07-08 17:02 ` Christian Marangi
0 siblings, 1 reply; 11+ messages in thread
From: Christian Marangi @ 2022-07-08 16:47 UTC (permalink / raw)
To: Robert Marko
Cc: Bjorn Helgaas, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
lpieralisi, Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1,
linux-pci, linux-arm-msm, open list, johan+linaro,
Dmitry Baryshkov
On Fri, Jul 08, 2022 at 06:39:37PM +0200, Robert Marko wrote:
> CC-ing Christian who did a lot of work on 2.1.0 (IPQ806x).
>
> Regards,
> Robert
>
> On Thu, 7 Jul 2022 at 21:41, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> > > IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> > > It's not clear whether other variants have the same dependency, but there
> > > seems to be no reason for them to be different, so move all the DBI
> > > accesses from .init() to .post_init() so they are all after phy_power_on().
> > >
> > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> >
> > Would any of the qcom driver folks care to review and ack this?
> > Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
> >
Hi Bjorn,
I tested this on ipq806x and the current patch cause regression as pci
doesn't work anymore...
This is a before the patch [1] and this is an after [2].
As you notice the main problem here is
[ 2.559962] qcom-pcie 1b700000.pci: Phy link never came up
The cause of this has already been bisected and actually it was a fixup
pushed some time ago for 2_1_0.
Uboot can leave the pci in an underfined state and this
writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
is never called.
This is mandatory to a correct init and MUST be called before regulator
enable and reset deassert or the "Phy link never came up" problem is
triggered.
So to fix this we just have to have
writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
in qcom_pcie_init_2_1_0 right after the reset_contro_assert.
This command is also present in qcom_pcie_init_2_3_2 where the same
exact reg is written so I assume 2_3_2 have the same regression and the
write must be placed in init and can't be moved to post_init.
Feel free to tell me how to proceed if I should post an additional patch
or you prefer Robi to respin this with the few lines reverted.
[1] https://gist.github.com/Ansuel/ec827319e585630356fc586273db6f0d
[2] https://gist.github.com/Ansuel/63fbcab2681cd28a61ec52d7874fa30d
> > > ---
> > > Changes in v4:
> > > * Move 2.7.0 accesses as well
> > > * Correct title and description (Bjorn)
> > > ---
> > > drivers/pci/controller/dwc/pcie-qcom.c | 215 ++++++++++++++-----------
> > > 1 file changed, 119 insertions(+), 96 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 24708d5d817d..f1a156052fe7 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -348,8 +348,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
> > > struct dw_pcie *pci = pcie->pci;
> > > struct device *dev = pci->dev;
> > > - struct device_node *node = dev->of_node;
> > > - u32 val;
> > > int ret;
> > >
> > > /* reset the PCIe interface as uboot can leave it undefined state */
> > > @@ -360,8 +358,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > reset_control_assert(res->ext_reset);
> > > reset_control_assert(res->phy_reset);
> > >
> > > - writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > -
> > > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > > if (ret < 0) {
> > > dev_err(dev, "cannot enable regulators\n");
> > > @@ -408,6 +404,35 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > if (ret)
> > > goto err_clks;
> > >
> > > + return 0;
> > > +
> > > +err_clks:
> > > + reset_control_assert(res->axi_reset);
> > > +err_deassert_axi:
> > > + reset_control_assert(res->por_reset);
> > > +err_deassert_por:
> > > + reset_control_assert(res->pci_reset);
> > > +err_deassert_pci:
> > > + reset_control_assert(res->phy_reset);
> > > +err_deassert_phy:
> > > + reset_control_assert(res->ext_reset);
> > > +err_deassert_ext:
> > > + reset_control_assert(res->ahb_reset);
> > > +err_deassert_ahb:
> > > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int qcom_pcie_post_init_2_1_0(struct qcom_pcie *pcie)
> > > +{
> > > + struct dw_pcie *pci = pcie->pci;
> > > + struct device *dev = pci->dev;
> > > + struct device_node *node = dev->of_node;
> > > + u32 val;
> > > +
> > > + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > +
> > > /* enable PCIe clocks and resets */
> > > val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > val &= ~BIT(0);
> > > @@ -451,23 +476,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
> > >
> > > return 0;
> > > -
> > > -err_clks:
> > > - reset_control_assert(res->axi_reset);
> > > -err_deassert_axi:
> > > - reset_control_assert(res->por_reset);
> > > -err_deassert_por:
> > > - reset_control_assert(res->pci_reset);
> > > -err_deassert_pci:
> > > - reset_control_assert(res->phy_reset);
> > > -err_deassert_phy:
> > > - reset_control_assert(res->ext_reset);
> > > -err_deassert_ext:
> > > - reset_control_assert(res->ahb_reset);
> > > -err_deassert_ahb:
> > > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > -
> > > - return ret;
> > > }
> > >
> > > static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
> > > @@ -555,16 +563,6 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> > > goto err_slave;
> > > }
> > >
> > > - /* change DBI base address */
> > > - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > -
> > > - if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > > - u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > -
> > > - val |= BIT(31);
> > > - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > - }
> > > -
> > > return 0;
> > > err_slave:
> > > clk_disable_unprepare(res->slave_bus);
> > > @@ -580,6 +578,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> > > return ret;
> > > }
> > >
> > > +static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> > > +{
> > > +
> > > + /* change DBI base address */
> > > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > +
> > > + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > > + u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > +
> > > + val |= BIT(31);
> > > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
> > > {
> > > u32 val;
> > > @@ -648,7 +662,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> > > struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> > > struct dw_pcie *pci = pcie->pci;
> > > struct device *dev = pci->dev;
> > > - u32 val;
> > > int ret;
> > >
> > > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > > @@ -681,27 +694,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> > > goto err_slave_clk;
> > > }
> > >
> > > - /* enable PCIe clocks and resets */
> > > - val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > - val &= ~BIT(0);
> > > - writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > -
> > > - /* change DBI base address */
> > > - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > -
> > > - /* MAC PHY_POWERDOWN MUX DISABLE */
> > > - val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > - val &= ~BIT(29);
> > > - writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > -
> > > - val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > - val |= BIT(4);
> > > - writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > -
> > > - val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > - val |= BIT(31);
> > > - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > -
> > > return 0;
> > >
> > > err_slave_clk:
> > > @@ -722,8 +714,30 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
> > > struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> > > struct dw_pcie *pci = pcie->pci;
> > > struct device *dev = pci->dev;
> > > + u32 val;
> > > int ret;
> > >
> > > + /* enable PCIe clocks and resets */
> > > + val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > + val &= ~BIT(0);
> > > + writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > +
> > > + /* change DBI base address */
> > > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > +
> > > + /* MAC PHY_POWERDOWN MUX DISABLE */
> > > + val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > + val &= ~BIT(29);
> > > + writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > +
> > > + val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > + val |= BIT(4);
> > > + writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > +
> > > + val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > + val |= BIT(31);
> > > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > +
> > > ret = clk_prepare_enable(res->pipe_clk);
> > > if (ret) {
> > > dev_err(dev, "cannot prepare/enable pipe clock\n");
> > > @@ -837,7 +851,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > > struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> > > struct dw_pcie *pci = pcie->pci;
> > > struct device *dev = pci->dev;
> > > - u32 val;
> > > int ret;
> > >
> > > ret = reset_control_assert(res->axi_m_reset);
> > > @@ -962,6 +975,33 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > > if (ret)
> > > goto err_clks;
> > >
> > > + return 0;
> > > +
> > > +err_clks:
> > > + reset_control_assert(res->ahb_reset);
> > > +err_rst_ahb:
> > > + reset_control_assert(res->pwr_reset);
> > > +err_rst_pwr:
> > > + reset_control_assert(res->axi_s_reset);
> > > +err_rst_axi_s:
> > > + reset_control_assert(res->axi_m_sticky_reset);
> > > +err_rst_axi_m_sticky:
> > > + reset_control_assert(res->axi_m_reset);
> > > +err_rst_axi_m:
> > > + reset_control_assert(res->pipe_sticky_reset);
> > > +err_rst_pipe_sticky:
> > > + reset_control_assert(res->pipe_reset);
> > > +err_rst_pipe:
> > > + reset_control_assert(res->phy_reset);
> > > +err_rst_phy:
> > > + reset_control_assert(res->phy_ahb_reset);
> > > + return ret;
> > > +}
> > > +
> > > +static int qcom_pcie_post_init_2_4_0(struct qcom_pcie *pcie)
> > > +{
> > > + u32 val;
> > > +
> > > /* enable PCIe clocks and resets */
> > > val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > val &= ~BIT(0);
> > > @@ -984,26 +1024,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > > writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > >
> > > return 0;
> > > -
> > > -err_clks:
> > > - reset_control_assert(res->ahb_reset);
> > > -err_rst_ahb:
> > > - reset_control_assert(res->pwr_reset);
> > > -err_rst_pwr:
> > > - reset_control_assert(res->axi_s_reset);
> > > -err_rst_axi_s:
> > > - reset_control_assert(res->axi_m_sticky_reset);
> > > -err_rst_axi_m_sticky:
> > > - reset_control_assert(res->axi_m_reset);
> > > -err_rst_axi_m:
> > > - reset_control_assert(res->pipe_sticky_reset);
> > > -err_rst_pipe_sticky:
> > > - reset_control_assert(res->pipe_reset);
> > > -err_rst_pipe:
> > > - reset_control_assert(res->phy_reset);
> > > -err_rst_phy:
> > > - reset_control_assert(res->phy_ahb_reset);
> > > - return ret;
> > > }
> > >
> > > static int qcom_pcie_get_resources_2_3_3(struct qcom_pcie *pcie)
> > > @@ -1237,7 +1257,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > > struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > struct dw_pcie *pci = pcie->pci;
> > > struct device *dev = pci->dev;
> > > - u32 val;
> > > int ret;
> > >
> > > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > > @@ -1271,6 +1290,28 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > > /* Wait for reset to complete, required on SM8450 */
> > > usleep_range(1000, 1500);
> > >
> > > + return 0;
> > > +err_disable_clocks:
> > > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > +err_disable_regulators:
> > > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > > +{
> > > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > +
> > > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > +}
> > > +
> > > +static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> > > +{
> > > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > + u32 val;
> > > +
> > > /* configure PCIe to RC mode */
> > > writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> > >
> > > @@ -1297,27 +1338,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > > writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > }
> > >
> > > - return 0;
> > > -err_disable_clocks:
> > > - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > -err_disable_regulators:
> > > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > -
> > > - return ret;
> > > -}
> > > -
> > > -static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > > -{
> > > - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > -
> > > - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > -}
> > > -
> > > -static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> > > -{
> > > - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > -
> > > /* Set pipe clock as clock source for pcie_pipe_clk_src */
> > > if (pcie->cfg->pipe_clk_need_muxing)
> > > clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
> > > @@ -1569,6 +1589,7 @@ static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> > > static const struct qcom_pcie_ops ops_2_1_0 = {
> > > .get_resources = qcom_pcie_get_resources_2_1_0,
> > > .init = qcom_pcie_init_2_1_0,
> > > + .post_init = qcom_pcie_post_init_2_1_0,
> > > .deinit = qcom_pcie_deinit_2_1_0,
> > > .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> > > };
> > > @@ -1577,6 +1598,7 @@ static const struct qcom_pcie_ops ops_2_1_0 = {
> > > static const struct qcom_pcie_ops ops_1_0_0 = {
> > > .get_resources = qcom_pcie_get_resources_1_0_0,
> > > .init = qcom_pcie_init_1_0_0,
> > > + .post_init = qcom_pcie_post_init_1_0_0,
> > > .deinit = qcom_pcie_deinit_1_0_0,
> > > .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> > > };
> > > @@ -1595,6 +1617,7 @@ static const struct qcom_pcie_ops ops_2_3_2 = {
> > > static const struct qcom_pcie_ops ops_2_4_0 = {
> > > .get_resources = qcom_pcie_get_resources_2_4_0,
> > > .init = qcom_pcie_init_2_4_0,
> > > + .post_init = qcom_pcie_post_init_2_4_0,
> > > .deinit = qcom_pcie_deinit_2_4_0,
> > > .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > > };
> > > --
> > > 2.36.1
> > >
--
Ansuel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-07-08 16:47 ` Christian Marangi
@ 2022-07-08 17:02 ` Christian Marangi
2022-07-08 19:17 ` Bjorn Helgaas
0 siblings, 1 reply; 11+ messages in thread
From: Christian Marangi @ 2022-07-08 17:02 UTC (permalink / raw)
To: Robert Marko
Cc: Bjorn Helgaas, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
lpieralisi, Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1,
linux-pci, linux-arm-msm, open list, johan+linaro,
Dmitry Baryshkov
On Fri, Jul 08, 2022 at 06:47:57PM +0200, Christian Marangi wrote:
> On Fri, Jul 08, 2022 at 06:39:37PM +0200, Robert Marko wrote:
> > CC-ing Christian who did a lot of work on 2.1.0 (IPQ806x).
> >
> > Regards,
> > Robert
> >
> > On Thu, 7 Jul 2022 at 21:41, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> > > > IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> > > > It's not clear whether other variants have the same dependency, but there
> > > > seems to be no reason for them to be different, so move all the DBI
> > > > accesses from .init() to .post_init() so they are all after phy_power_on().
> > > >
> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > >
> > > Would any of the qcom driver folks care to review and ack this?
> > > Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
> > >
>
> Hi Bjorn,
> I tested this on ipq806x and the current patch cause regression as pci
> doesn't work anymore...
> This is a before the patch [1] and this is an after [2].
>
> As you notice the main problem here is
> [ 2.559962] qcom-pcie 1b700000.pci: Phy link never came up
>
> The cause of this has already been bisected and actually it was a fixup
> pushed some time ago for 2_1_0.
>
> Uboot can leave the pci in an underfined state and this
> writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> is never called.
>
> This is mandatory to a correct init and MUST be called before regulator
> enable and reset deassert or the "Phy link never came up" problem is
> triggered.
>
> So to fix this we just have to have
> writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> in qcom_pcie_init_2_1_0 right after the reset_contro_assert.
>
> This command is also present in qcom_pcie_init_2_3_2 where the same
> exact reg is written so I assume 2_3_2 have the same regression and the
> write must be placed in init and can't be moved to post_init.
>
> Feel free to tell me how to proceed if I should post an additional patch
> or you prefer Robi to respin this with the few lines reverted.
>
> [1] https://gist.github.com/Ansuel/ec827319e585630356fc586273db6f0d
> [2] https://gist.github.com/Ansuel/63fbcab2681cd28a61ec52d7874fa30d
>
While testing this I notice something odd...
2_4_2 prepare the pipe clock only AFTER PCIe clocks and reset are
enabled while in 2_1_0... That made me think there could be a problem
with the current code of 2_1_0... A quick change made me discover that
the problem is actually that we enable prepare_enable clock BEFORE the
value is written in PCIE20_PARF_PHY_CTRL.
By moving the clk_bulk_prepare_enable after the "enable PCIe clocks and
resets" make the pci work with the current change...
So it could be that the current changes are correct and it's really just
a bug in 2_1_0 enabling clock before writing the correct value...
Tell me how to proceed... think at this point a good idea would be to
create a separate patch and fix this for good.
Also bonus question, should I drop the bulk_prepare_enable and follow
the pattern of 2_3_2 and enable aux, cfg, master and slave in init and
pipe in post init or I can keep it? (still have to test but I assume
that it will work.)
> > > > ---
> > > > Changes in v4:
> > > > * Move 2.7.0 accesses as well
> > > > * Correct title and description (Bjorn)
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-qcom.c | 215 ++++++++++++++-----------
> > > > 1 file changed, 119 insertions(+), 96 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 24708d5d817d..f1a156052fe7 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -348,8 +348,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > > struct qcom_pcie_resources_2_1_0 *res = &pcie->res.v2_1_0;
> > > > struct dw_pcie *pci = pcie->pci;
> > > > struct device *dev = pci->dev;
> > > > - struct device_node *node = dev->of_node;
> > > > - u32 val;
> > > > int ret;
> > > >
> > > > /* reset the PCIe interface as uboot can leave it undefined state */
> > > > @@ -360,8 +358,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > > reset_control_assert(res->ext_reset);
> > > > reset_control_assert(res->phy_reset);
> > > >
> > > > - writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > -
> > > > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > if (ret < 0) {
> > > > dev_err(dev, "cannot enable regulators\n");
> > > > @@ -408,6 +404,35 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > > if (ret)
> > > > goto err_clks;
> > > >
> > > > + return 0;
> > > > +
> > > > +err_clks:
> > > > + reset_control_assert(res->axi_reset);
> > > > +err_deassert_axi:
> > > > + reset_control_assert(res->por_reset);
> > > > +err_deassert_por:
> > > > + reset_control_assert(res->pci_reset);
> > > > +err_deassert_pci:
> > > > + reset_control_assert(res->phy_reset);
> > > > +err_deassert_phy:
> > > > + reset_control_assert(res->ext_reset);
> > > > +err_deassert_ext:
> > > > + reset_control_assert(res->ahb_reset);
> > > > +err_deassert_ahb:
> > > > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int qcom_pcie_post_init_2_1_0(struct qcom_pcie *pcie)
> > > > +{
> > > > + struct dw_pcie *pci = pcie->pci;
> > > > + struct device *dev = pci->dev;
> > > > + struct device_node *node = dev->of_node;
> > > > + u32 val;
> > > > +
> > > > + writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > +
> > > > /* enable PCIe clocks and resets */
> > > > val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > val &= ~BIT(0);
> > > > @@ -451,23 +476,6 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
> > > > pci->dbi_base + PCIE20_AXI_MSTR_RESP_COMP_CTRL1);
> > > >
> > > > return 0;
> > > > -
> > > > -err_clks:
> > > > - reset_control_assert(res->axi_reset);
> > > > -err_deassert_axi:
> > > > - reset_control_assert(res->por_reset);
> > > > -err_deassert_por:
> > > > - reset_control_assert(res->pci_reset);
> > > > -err_deassert_pci:
> > > > - reset_control_assert(res->phy_reset);
> > > > -err_deassert_phy:
> > > > - reset_control_assert(res->ext_reset);
> > > > -err_deassert_ext:
> > > > - reset_control_assert(res->ahb_reset);
> > > > -err_deassert_ahb:
> > > > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > -
> > > > - return ret;
> > > > }
> > > >
> > > > static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
> > > > @@ -555,16 +563,6 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> > > > goto err_slave;
> > > > }
> > > >
> > > > - /* change DBI base address */
> > > > - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > > -
> > > > - if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > > > - u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > > -
> > > > - val |= BIT(31);
> > > > - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > > - }
> > > > -
> > > > return 0;
> > > > err_slave:
> > > > clk_disable_unprepare(res->slave_bus);
> > > > @@ -580,6 +578,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
> > > > return ret;
> > > > }
> > > >
> > > > +static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> > > > +{
> > > > +
> > > > + /* change DBI base address */
> > > > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > > +
> > > > + if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > > > + u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > > +
> > > > + val |= BIT(31);
> > > > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
> > > > {
> > > > u32 val;
> > > > @@ -648,7 +662,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> > > > struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> > > > struct dw_pcie *pci = pcie->pci;
> > > > struct device *dev = pci->dev;
> > > > - u32 val;
> > > > int ret;
> > > >
> > > > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > @@ -681,27 +694,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
> > > > goto err_slave_clk;
> > > > }
> > > >
> > > > - /* enable PCIe clocks and resets */
> > > > - val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > - val &= ~BIT(0);
> > > > - writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > -
> > > > - /* change DBI base address */
> > > > - writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > > -
> > > > - /* MAC PHY_POWERDOWN MUX DISABLE */
> > > > - val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > > - val &= ~BIT(29);
> > > > - writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > > -
> > > > - val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > > - val |= BIT(4);
> > > > - writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > > -
> > > > - val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > > - val |= BIT(31);
> > > > - writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > > -
> > > > return 0;
> > > >
> > > > err_slave_clk:
> > > > @@ -722,8 +714,30 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
> > > > struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
> > > > struct dw_pcie *pci = pcie->pci;
> > > > struct device *dev = pci->dev;
> > > > + u32 val;
> > > > int ret;
> > > >
> > > > + /* enable PCIe clocks and resets */
> > > > + val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > + val &= ~BIT(0);
> > > > + writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > +
> > > > + /* change DBI base address */
> > > > + writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> > > > +
> > > > + /* MAC PHY_POWERDOWN MUX DISABLE */
> > > > + val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > > + val &= ~BIT(29);
> > > > + writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> > > > +
> > > > + val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > > + val |= BIT(4);
> > > > + writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> > > > +
> > > > + val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > > + val |= BIT(31);
> > > > + writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > > +
> > > > ret = clk_prepare_enable(res->pipe_clk);
> > > > if (ret) {
> > > > dev_err(dev, "cannot prepare/enable pipe clock\n");
> > > > @@ -837,7 +851,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > > > struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> > > > struct dw_pcie *pci = pcie->pci;
> > > > struct device *dev = pci->dev;
> > > > - u32 val;
> > > > int ret;
> > > >
> > > > ret = reset_control_assert(res->axi_m_reset);
> > > > @@ -962,6 +975,33 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > > > if (ret)
> > > > goto err_clks;
> > > >
> > > > + return 0;
> > > > +
> > > > +err_clks:
> > > > + reset_control_assert(res->ahb_reset);
> > > > +err_rst_ahb:
> > > > + reset_control_assert(res->pwr_reset);
> > > > +err_rst_pwr:
> > > > + reset_control_assert(res->axi_s_reset);
> > > > +err_rst_axi_s:
> > > > + reset_control_assert(res->axi_m_sticky_reset);
> > > > +err_rst_axi_m_sticky:
> > > > + reset_control_assert(res->axi_m_reset);
> > > > +err_rst_axi_m:
> > > > + reset_control_assert(res->pipe_sticky_reset);
> > > > +err_rst_pipe_sticky:
> > > > + reset_control_assert(res->pipe_reset);
> > > > +err_rst_pipe:
> > > > + reset_control_assert(res->phy_reset);
> > > > +err_rst_phy:
> > > > + reset_control_assert(res->phy_ahb_reset);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int qcom_pcie_post_init_2_4_0(struct qcom_pcie *pcie)
> > > > +{
> > > > + u32 val;
> > > > +
> > > > /* enable PCIe clocks and resets */
> > > > val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > val &= ~BIT(0);
> > > > @@ -984,26 +1024,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > > > writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> > > >
> > > > return 0;
> > > > -
> > > > -err_clks:
> > > > - reset_control_assert(res->ahb_reset);
> > > > -err_rst_ahb:
> > > > - reset_control_assert(res->pwr_reset);
> > > > -err_rst_pwr:
> > > > - reset_control_assert(res->axi_s_reset);
> > > > -err_rst_axi_s:
> > > > - reset_control_assert(res->axi_m_sticky_reset);
> > > > -err_rst_axi_m_sticky:
> > > > - reset_control_assert(res->axi_m_reset);
> > > > -err_rst_axi_m:
> > > > - reset_control_assert(res->pipe_sticky_reset);
> > > > -err_rst_pipe_sticky:
> > > > - reset_control_assert(res->pipe_reset);
> > > > -err_rst_pipe:
> > > > - reset_control_assert(res->phy_reset);
> > > > -err_rst_phy:
> > > > - reset_control_assert(res->phy_ahb_reset);
> > > > - return ret;
> > > > }
> > > >
> > > > static int qcom_pcie_get_resources_2_3_3(struct qcom_pcie *pcie)
> > > > @@ -1237,7 +1257,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > > > struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > > struct dw_pcie *pci = pcie->pci;
> > > > struct device *dev = pci->dev;
> > > > - u32 val;
> > > > int ret;
> > > >
> > > > ret = regulator_bulk_enable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > @@ -1271,6 +1290,28 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > > > /* Wait for reset to complete, required on SM8450 */
> > > > usleep_range(1000, 1500);
> > > >
> > > > + return 0;
> > > > +err_disable_clocks:
> > > > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > > +err_disable_regulators:
> > > > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > > > +{
> > > > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > > +
> > > > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > > + regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > +}
> > > > +
> > > > +static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> > > > +{
> > > > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > > + u32 val;
> > > > +
> > > > /* configure PCIe to RC mode */
> > > > writel(DEVICE_TYPE_RC, pcie->parf + PCIE20_PARF_DEVICE_TYPE);
> > > >
> > > > @@ -1297,27 +1338,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> > > > writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> > > > }
> > > >
> > > > - return 0;
> > > > -err_disable_clocks:
> > > > - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > > -err_disable_regulators:
> > > > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > -
> > > > - return ret;
> > > > -}
> > > > -
> > > > -static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > > > -{
> > > > - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > > -
> > > > - clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > > - regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > > -}
> > > > -
> > > > -static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
> > > > -{
> > > > - struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > > -
> > > > /* Set pipe clock as clock source for pcie_pipe_clk_src */
> > > > if (pcie->cfg->pipe_clk_need_muxing)
> > > > clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
> > > > @@ -1569,6 +1589,7 @@ static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> > > > static const struct qcom_pcie_ops ops_2_1_0 = {
> > > > .get_resources = qcom_pcie_get_resources_2_1_0,
> > > > .init = qcom_pcie_init_2_1_0,
> > > > + .post_init = qcom_pcie_post_init_2_1_0,
> > > > .deinit = qcom_pcie_deinit_2_1_0,
> > > > .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> > > > };
> > > > @@ -1577,6 +1598,7 @@ static const struct qcom_pcie_ops ops_2_1_0 = {
> > > > static const struct qcom_pcie_ops ops_1_0_0 = {
> > > > .get_resources = qcom_pcie_get_resources_1_0_0,
> > > > .init = qcom_pcie_init_1_0_0,
> > > > + .post_init = qcom_pcie_post_init_1_0_0,
> > > > .deinit = qcom_pcie_deinit_1_0_0,
> > > > .ltssm_enable = qcom_pcie_2_1_0_ltssm_enable,
> > > > };
> > > > @@ -1595,6 +1617,7 @@ static const struct qcom_pcie_ops ops_2_3_2 = {
> > > > static const struct qcom_pcie_ops ops_2_4_0 = {
> > > > .get_resources = qcom_pcie_get_resources_2_4_0,
> > > > .init = qcom_pcie_init_2_4_0,
> > > > + .post_init = qcom_pcie_post_init_2_4_0,
> > > > .deinit = qcom_pcie_deinit_2_4_0,
> > > > .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > > > };
> > > > --
> > > > 2.36.1
> > > >
>
> --
> Ansuel
--
Ansuel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-07-08 17:02 ` Christian Marangi
@ 2022-07-08 19:17 ` Bjorn Helgaas
2022-07-08 19:22 ` Christian Marangi
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2022-07-08 19:17 UTC (permalink / raw)
To: Christian Marangi
Cc: Robert Marko, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
lpieralisi, Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1,
linux-pci, linux-arm-msm, open list, johan+linaro,
Dmitry Baryshkov
On Fri, Jul 08, 2022 at 07:02:48PM +0200, Christian Marangi wrote:
> On Fri, Jul 08, 2022 at 06:47:57PM +0200, Christian Marangi wrote:
> > On Fri, Jul 08, 2022 at 06:39:37PM +0200, Robert Marko wrote:
> > > On Thu, 7 Jul 2022 at 21:41, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> > > > > IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> > > > > It's not clear whether other variants have the same dependency, but there
> > > > > seems to be no reason for them to be different, so move all the DBI
> > > > > accesses from .init() to .post_init() so they are all after phy_power_on().
> > > > >
> > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > >
> > > > Would any of the qcom driver folks care to review and ack this?
> > > > Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
> >
> > Hi Bjorn,
> > I tested this on ipq806x and the current patch cause regression as pci
> > doesn't work anymore...
> > This is a before the patch [1] and this is an after [2].
> >
> > As you notice the main problem here is
> > [ 2.559962] qcom-pcie 1b700000.pci: Phy link never came up
> >
> > The cause of this has already been bisected and actually it was a fixup
> > pushed some time ago for 2_1_0.
> >
> > Uboot can leave the pci in an underfined state and this
> > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > is never called.
> >
> > This is mandatory to a correct init and MUST be called before regulator
> > enable and reset deassert or the "Phy link never came up" problem is
> > triggered.
> >
> > So to fix this we just have to have
> > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > in qcom_pcie_init_2_1_0 right after the reset_contro_assert.
> >
> > This command is also present in qcom_pcie_init_2_3_2 where the same
> > exact reg is written so I assume 2_3_2 have the same regression and the
> > write must be placed in init and can't be moved to post_init.
> >
> > Feel free to tell me how to proceed if I should post an additional patch
> > or you prefer Robi to respin this with the few lines reverted.
> >
> > [1] https://gist.github.com/Ansuel/ec827319e585630356fc586273db6f0d
> > [2] https://gist.github.com/Ansuel/63fbcab2681cd28a61ec52d7874fa30d
>
> While testing this I notice something odd...
>
> 2_4_2 prepare the pipe clock only AFTER PCIe clocks and reset are
> enabled while in 2_1_0... That made me think there could be a problem
> with the current code of 2_1_0... A quick change made me discover that
> the problem is actually that we enable prepare_enable clock BEFORE the
> value is written in PCIE20_PARF_PHY_CTRL.
>
> By moving the clk_bulk_prepare_enable after the "enable PCIe clocks and
> resets" make the pci work with the current change...
>
> So it could be that the current changes are correct and it's really just
> a bug in 2_1_0 enabling clock before writing the correct value...
>
> Tell me how to proceed... think at this point a good idea would be to
> create a separate patch and fix this for good.
Hmm, I think I made a mistake when I put this patch in the middle and
applied other stuff on top of it. I'd like to just postpone this
patch while we work out these issues, but I think it's not completely
trivial since it's in the middle. I'll try to straighten this out
next week.
> Also bonus question, should I drop the bulk_prepare_enable and follow
> the pattern of 2_3_2 and enable aux, cfg, master and slave in init and
> pipe in post init or I can keep it? (still have to test but I assume
> that it will work.)
I haven't looked at the code again, but will just offer the opinion
that unnecessary differences in structure often hide bugs, and they're
always minor speed bumps for readers.
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-07-08 19:17 ` Bjorn Helgaas
@ 2022-07-08 19:22 ` Christian Marangi
2022-07-08 20:11 ` Bjorn Helgaas
0 siblings, 1 reply; 11+ messages in thread
From: Christian Marangi @ 2022-07-08 19:22 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Robert Marko, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
lpieralisi, Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1,
linux-pci, linux-arm-msm, open list, johan+linaro,
Dmitry Baryshkov
On Fri, Jul 08, 2022 at 02:17:09PM -0500, Bjorn Helgaas wrote:
> On Fri, Jul 08, 2022 at 07:02:48PM +0200, Christian Marangi wrote:
> > On Fri, Jul 08, 2022 at 06:47:57PM +0200, Christian Marangi wrote:
> > > On Fri, Jul 08, 2022 at 06:39:37PM +0200, Robert Marko wrote:
> > > > On Thu, 7 Jul 2022 at 21:41, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> > > > > > IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> > > > > > It's not clear whether other variants have the same dependency, but there
> > > > > > seems to be no reason for them to be different, so move all the DBI
> > > > > > accesses from .init() to .post_init() so they are all after phy_power_on().
> > > > > >
> > > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > >
> > > > > Would any of the qcom driver folks care to review and ack this?
> > > > > Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
> > >
> > > Hi Bjorn,
> > > I tested this on ipq806x and the current patch cause regression as pci
> > > doesn't work anymore...
> > > This is a before the patch [1] and this is an after [2].
> > >
> > > As you notice the main problem here is
> > > [ 2.559962] qcom-pcie 1b700000.pci: Phy link never came up
> > >
> > > The cause of this has already been bisected and actually it was a fixup
> > > pushed some time ago for 2_1_0.
> > >
> > > Uboot can leave the pci in an underfined state and this
> > > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > is never called.
> > >
> > > This is mandatory to a correct init and MUST be called before regulator
> > > enable and reset deassert or the "Phy link never came up" problem is
> > > triggered.
> > >
> > > So to fix this we just have to have
> > > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > in qcom_pcie_init_2_1_0 right after the reset_contro_assert.
> > >
> > > This command is also present in qcom_pcie_init_2_3_2 where the same
> > > exact reg is written so I assume 2_3_2 have the same regression and the
> > > write must be placed in init and can't be moved to post_init.
> > >
> > > Feel free to tell me how to proceed if I should post an additional patch
> > > or you prefer Robi to respin this with the few lines reverted.
> > >
> > > [1] https://gist.github.com/Ansuel/ec827319e585630356fc586273db6f0d
> > > [2] https://gist.github.com/Ansuel/63fbcab2681cd28a61ec52d7874fa30d
> >
> > While testing this I notice something odd...
> >
> > 2_4_2 prepare the pipe clock only AFTER PCIe clocks and reset are
> > enabled while in 2_1_0... That made me think there could be a problem
> > with the current code of 2_1_0... A quick change made me discover that
> > the problem is actually that we enable prepare_enable clock BEFORE the
> > value is written in PCIE20_PARF_PHY_CTRL.
> >
> > By moving the clk_bulk_prepare_enable after the "enable PCIe clocks and
> > resets" make the pci work with the current change...
> >
> > So it could be that the current changes are correct and it's really just
> > a bug in 2_1_0 enabling clock before writing the correct value...
> >
> > Tell me how to proceed... think at this point a good idea would be to
> > create a separate patch and fix this for good.
>
> Hmm, I think I made a mistake when I put this patch in the middle and
> applied other stuff on top of it. I'd like to just postpone this
> patch while we work out these issues, but I think it's not completely
> trivial since it's in the middle. I'll try to straighten this out
> next week.
>
From my discoveries it really seems just a bug in 2_1_0 with enabling
the phy clk BEFORE setting the require bit...
Moving the bulk_prepare_enable after the bit is set makes everything
works as it should... If you want I can send a patch as that is clearly
a bug and currenty we have a workaround in place...
(with the patch the workaround can be dropped aka we even remove a line
of code)
> > Also bonus question, should I drop the bulk_prepare_enable and follow
> > the pattern of 2_3_2 and enable aux, cfg, master and slave in init and
> > pipe in post init or I can keep it? (still have to test but I assume
> > that it will work.)
>
> I haven't looked at the code again, but will just offer the opinion
> that unnecessary differences in structure often hide bugs, and they're
> always minor speed bumps for readers.
>
> Bjorn
Considering it won't change anything and pci 2_1_0 use the bulk api...
calling each clock separately will actually makes things less
readable... Sooo think it's just a line to drop and a code move.
--
Ansuel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-07-08 19:22 ` Christian Marangi
@ 2022-07-08 20:11 ` Bjorn Helgaas
2022-07-08 22:28 ` Christian Marangi
0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Helgaas @ 2022-07-08 20:11 UTC (permalink / raw)
To: Christian Marangi
Cc: Robert Marko, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
lpieralisi, Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1,
linux-pci, linux-arm-msm, open list, johan+linaro,
Dmitry Baryshkov
On Fri, Jul 08, 2022 at 09:22:09PM +0200, Christian Marangi wrote:
> On Fri, Jul 08, 2022 at 02:17:09PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jul 08, 2022 at 07:02:48PM +0200, Christian Marangi wrote:
> > > On Fri, Jul 08, 2022 at 06:47:57PM +0200, Christian Marangi wrote:
> > > > On Fri, Jul 08, 2022 at 06:39:37PM +0200, Robert Marko wrote:
> > > > > On Thu, 7 Jul 2022 at 21:41, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> > > > > > > IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> > > > > > > It's not clear whether other variants have the same dependency, but there
> > > > > > > seems to be no reason for them to be different, so move all the DBI
> > > > > > > accesses from .init() to .post_init() so they are all after phy_power_on().
> > > > > > >
> > > > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > > >
> > > > > > Would any of the qcom driver folks care to review and ack this?
> > > > > > Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
> > > >
> > > > Hi Bjorn,
> > > > I tested this on ipq806x and the current patch cause regression as pci
> > > > doesn't work anymore...
> > > > This is a before the patch [1] and this is an after [2].
> > > >
> > > > As you notice the main problem here is
> > > > [ 2.559962] qcom-pcie 1b700000.pci: Phy link never came up
> > > >
> > > > The cause of this has already been bisected and actually it was a fixup
> > > > pushed some time ago for 2_1_0.
> > > >
> > > > Uboot can leave the pci in an underfined state and this
> > > > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > is never called.
> > > >
> > > > This is mandatory to a correct init and MUST be called before regulator
> > > > enable and reset deassert or the "Phy link never came up" problem is
> > > > triggered.
> > > >
> > > > So to fix this we just have to have
> > > > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > in qcom_pcie_init_2_1_0 right after the reset_contro_assert.
> > > >
> > > > This command is also present in qcom_pcie_init_2_3_2 where the same
> > > > exact reg is written so I assume 2_3_2 have the same regression and the
> > > > write must be placed in init and can't be moved to post_init.
> > > >
> > > > Feel free to tell me how to proceed if I should post an additional patch
> > > > or you prefer Robi to respin this with the few lines reverted.
> > > >
> > > > [1] https://gist.github.com/Ansuel/ec827319e585630356fc586273db6f0d
> > > > [2] https://gist.github.com/Ansuel/63fbcab2681cd28a61ec52d7874fa30d
> > >
> > > While testing this I notice something odd...
> > >
> > > 2_4_2 prepare the pipe clock only AFTER PCIe clocks and reset are
> > > enabled while in 2_1_0... That made me think there could be a problem
> > > with the current code of 2_1_0... A quick change made me discover that
> > > the problem is actually that we enable prepare_enable clock BEFORE the
> > > value is written in PCIE20_PARF_PHY_CTRL.
> > >
> > > By moving the clk_bulk_prepare_enable after the "enable PCIe clocks and
> > > resets" make the pci work with the current change...
> > >
> > > So it could be that the current changes are correct and it's really just
> > > a bug in 2_1_0 enabling clock before writing the correct value...
> > >
> > > Tell me how to proceed... think at this point a good idea would be to
> > > create a separate patch and fix this for good.
> >
> > Hmm, I think I made a mistake when I put this patch in the middle and
> > applied other stuff on top of it. I'd like to just postpone this
> > patch while we work out these issues, but I think it's not completely
> > trivial since it's in the middle. I'll try to straighten this out
> > next week.
>
> From my discoveries it really seems just a bug in 2_1_0 with enabling
> the phy clk BEFORE setting the require bit...
>
> Moving the bulk_prepare_enable after the bit is set makes everything
> works as it should... If you want I can send a patch as that is clearly
> a bug and currenty we have a workaround in place...
That'd be great! Since it's an actual bug fix, I think it would be
good if it were a separate patch instead of doing in the middle of a
patch that also does other things.
Bjorn
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] PCI: qcom: Move all DBI register accesses after phy_power_on()
2022-07-08 20:11 ` Bjorn Helgaas
@ 2022-07-08 22:28 ` Christian Marangi
0 siblings, 0 replies; 11+ messages in thread
From: Christian Marangi @ 2022-07-08 22:28 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Robert Marko, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
lpieralisi, Rob Herring, kw, Bjorn Helgaas, p.zabel, jingoohan1,
linux-pci, linux-arm-msm, open list, johan+linaro,
Dmitry Baryshkov
On Fri, Jul 08, 2022 at 03:11:25PM -0500, Bjorn Helgaas wrote:
> On Fri, Jul 08, 2022 at 09:22:09PM +0200, Christian Marangi wrote:
> > On Fri, Jul 08, 2022 at 02:17:09PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Jul 08, 2022 at 07:02:48PM +0200, Christian Marangi wrote:
> > > > On Fri, Jul 08, 2022 at 06:47:57PM +0200, Christian Marangi wrote:
> > > > > On Fri, Jul 08, 2022 at 06:39:37PM +0200, Robert Marko wrote:
> > > > > > On Thu, 7 Jul 2022 at 21:41, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > > > > On Fri, Jun 24, 2022 at 12:44:20PM +0200, Robert Marko wrote:
> > > > > > > > IPQ8074 requires the PHY to be powered on before accessing DBI registers.
> > > > > > > > It's not clear whether other variants have the same dependency, but there
> > > > > > > > seems to be no reason for them to be different, so move all the DBI
> > > > > > > > accesses from .init() to .post_init() so they are all after phy_power_on().
> > > > > > > >
> > > > > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > > > >
> > > > > > > Would any of the qcom driver folks care to review and ack this?
> > > > > > > Stanimir, Andy, Bjorn A (from get_maintainer.pl)?
> > > > >
> > > > > Hi Bjorn,
> > > > > I tested this on ipq806x and the current patch cause regression as pci
> > > > > doesn't work anymore...
> > > > > This is a before the patch [1] and this is an after [2].
> > > > >
> > > > > As you notice the main problem here is
> > > > > [ 2.559962] qcom-pcie 1b700000.pci: Phy link never came up
> > > > >
> > > > > The cause of this has already been bisected and actually it was a fixup
> > > > > pushed some time ago for 2_1_0.
> > > > >
> > > > > Uboot can leave the pci in an underfined state and this
> > > > > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > > is never called.
> > > > >
> > > > > This is mandatory to a correct init and MUST be called before regulator
> > > > > enable and reset deassert or the "Phy link never came up" problem is
> > > > > triggered.
> > > > >
> > > > > So to fix this we just have to have
> > > > > writel(1, pcie->parf + PCIE20_PARF_PHY_CTRL);
> > > > > in qcom_pcie_init_2_1_0 right after the reset_contro_assert.
> > > > >
> > > > > This command is also present in qcom_pcie_init_2_3_2 where the same
> > > > > exact reg is written so I assume 2_3_2 have the same regression and the
> > > > > write must be placed in init and can't be moved to post_init.
> > > > >
> > > > > Feel free to tell me how to proceed if I should post an additional patch
> > > > > or you prefer Robi to respin this with the few lines reverted.
> > > > >
> > > > > [1] https://gist.github.com/Ansuel/ec827319e585630356fc586273db6f0d
> > > > > [2] https://gist.github.com/Ansuel/63fbcab2681cd28a61ec52d7874fa30d
> > > >
> > > > While testing this I notice something odd...
> > > >
> > > > 2_4_2 prepare the pipe clock only AFTER PCIe clocks and reset are
> > > > enabled while in 2_1_0... That made me think there could be a problem
> > > > with the current code of 2_1_0... A quick change made me discover that
> > > > the problem is actually that we enable prepare_enable clock BEFORE the
> > > > value is written in PCIE20_PARF_PHY_CTRL.
> > > >
> > > > By moving the clk_bulk_prepare_enable after the "enable PCIe clocks and
> > > > resets" make the pci work with the current change...
> > > >
> > > > So it could be that the current changes are correct and it's really just
> > > > a bug in 2_1_0 enabling clock before writing the correct value...
> > > >
> > > > Tell me how to proceed... think at this point a good idea would be to
> > > > create a separate patch and fix this for good.
> > >
> > > Hmm, I think I made a mistake when I put this patch in the middle and
> > > applied other stuff on top of it. I'd like to just postpone this
> > > patch while we work out these issues, but I think it's not completely
> > > trivial since it's in the middle. I'll try to straighten this out
> > > next week.
> >
> > From my discoveries it really seems just a bug in 2_1_0 with enabling
> > the phy clk BEFORE setting the require bit...
> >
> > Moving the bulk_prepare_enable after the bit is set makes everything
> > works as it should... If you want I can send a patch as that is clearly
> > a bug and currenty we have a workaround in place...
>
> That'd be great! Since it's an actual bug fix, I think it would be
> good if it were a separate patch instead of doing in the middle of a
> patch that also does other things.
>
> Bjorn
Just sent the small fix that will indirectly make this series work as it
should.
--
Ansuel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-07-08 22:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-24 10:44 [PATCH v4 1/2] PCI: qcom: Move IPQ8074 DBI register accesses after phy_power_on() Robert Marko
2022-06-24 10:44 ` [PATCH v4 2/2] PCI: qcom: Move all " Robert Marko
2022-07-07 19:41 ` Bjorn Helgaas
2022-07-08 16:39 ` Robert Marko
2022-07-08 16:47 ` Christian Marangi
2022-07-08 17:02 ` Christian Marangi
2022-07-08 19:17 ` Bjorn Helgaas
2022-07-08 19:22 ` Christian Marangi
2022-07-08 20:11 ` Bjorn Helgaas
2022-07-08 22:28 ` Christian Marangi
2022-06-29 9:21 ` [PATCH v4 1/2] PCI: qcom: Move IPQ8074 " Robert Marko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).