| Message ID | 20220621112330.448754-1-robimarko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] PCI: qcom: fix IPQ8074 Gen2 support | expand |
On Tue, 21 Jun 2022 at 14:23, Robert Marko <robimarko@gmail.com> wrote: > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > cause the system to hang as its using DBI registers in the .init > and those are only accesible after phy_power_on(). > > So solve this by splitting the DBI read/writes to .post_init. > > Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code") Any elaboration for the Fixes tag? I think the follow one is more logical, isn't it? Fixes: 5d76117f070d ("PCI: qcom: Add support for IPQ8074 PCIe controller") > Signed-off-by: Robert Marko <robimarko@gmail.com> > --- > 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 51fed83484af..da6d79d61397 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) > @@ -1598,6 +1605,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 >
On Tue, 21 Jun 2022 at 19:29, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, 21 Jun 2022 at 14:23, Robert Marko <robimarko@gmail.com> wrote: > > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > cause the system to hang as its using DBI registers in the .init > > and those are only accesible after phy_power_on(). > > > > So solve this by splitting the DBI read/writes to .post_init. > > > > Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code") > > Any elaboration for the Fixes tag? I think the follow one is more > logical, isn't it? > > Fixes: 5d76117f070d ("PCI: qcom: Add support for IPQ8074 PCIe controller") Hi, My logic was that it was working before the commit a0fd361db8e5 as it moved PHY init later and indirectly broke IPQ8074 gen2. Regards, Robert > > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > --- > > 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 51fed83484af..da6d79d61397 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) > > @@ -1598,6 +1605,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 > > > > > -- > With best wishes > Dmitry
On 21/06/2022 21:53, Robert Marko wrote: > On Tue, 21 Jun 2022 at 19:29, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: >> >> On Tue, 21 Jun 2022 at 14:23, Robert Marko <robimarko@gmail.com> wrote: >>> >>> IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will >>> cause the system to hang as its using DBI registers in the .init >>> and those are only accesible after phy_power_on(). >>> >>> So solve this by splitting the DBI read/writes to .post_init. >>> >>> Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code") >> >> Any elaboration for the Fixes tag? I think the follow one is more >> logical, isn't it? >> >> Fixes: 5d76117f070d ("PCI: qcom: Add support for IPQ8074 PCIe controller") > > Hi, > My logic was that it was working before the commit a0fd361db8e5 as it > moved PHY init > later and indirectly broke IPQ8074 gen2. Ack, thanks for the explanation. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Regards, > Robert >> >>> Signed-off-by: Robert Marko <robimarko@gmail.com> >>> --- >>> 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 51fed83484af..da6d79d61397 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) >>> @@ -1598,6 +1605,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 >>> >> >> >> -- >> With best wishes >> Dmitry
On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > cause the system to hang as its using DBI registers in the .init > and those are only accesible after phy_power_on(). Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to this patch? I don't see the connection. I see that qcom_pcie_host_init() does: qcom_pcie_host_init pcie->cfg->ops->init(pcie) phy_power_on(pcie->phy) pcie->cfg->ops->post_init(pcie) and that you're moving DBI register accesses from qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). But I also see DBI register accesses in other .init() functions: qcom_pcie_init_2_1_0 qcom_pcie_init_1_0_0 (oddly out of order) qcom_pcie_init_2_3_2 qcom_pcie_init_2_4_0 Why do these accesses not need to be moved? I assume it's because pcie->phy is an optional PHY and phy_power_on() does nothing on those controllers? Whatever the reason, I think the DBI accesses should be done consistently in .post_init(). I see that Dmitry's previous patches removed all those .post_init() functions, but I think the consistency is worth having. Perhaps we could reorder the patches so this patch comes first, moves the DBI accesses into .post_init(), then Dmitry's patches could be rebased on top to drop the clock handling? > So solve this by splitting the DBI read/writes to .post_init. > > Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code") > Signed-off-by: Robert Marko <robimarko@gmail.com> > --- > 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 51fed83484af..da6d79d61397 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) > @@ -1598,6 +1605,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 >
On Tue, 21 Jun 2022 at 23:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > cause the system to hang as its using DBI registers in the .init > > and those are only accesible after phy_power_on(). > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > this patch? I don't see the connection. > > I see that qcom_pcie_host_init() does: > > qcom_pcie_host_init > pcie->cfg->ops->init(pcie) > phy_power_on(pcie->phy) > pcie->cfg->ops->post_init(pcie) > > and that you're moving DBI register accesses from > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > But I also see DBI register accesses in other .init() functions: > > qcom_pcie_init_2_1_0 > qcom_pcie_init_1_0_0 (oddly out of order) > qcom_pcie_init_2_3_2 > qcom_pcie_init_2_4_0 > > Why do these accesses not need to be moved? I assume it's because > pcie->phy is an optional PHY and phy_power_on() does nothing on those > controllers? > > Whatever the reason, I think the DBI accesses should be done > consistently in .post_init(). I see that Dmitry's previous patches > removed all those .post_init() functions, but I think the consistency > is worth having. > > Perhaps we could reorder the patches so this patch comes first, moves > the DBI accesses into .post_init(), then Dmitry's patches could be > rebased on top to drop the clock handling? I don't think there is a need to reorder patches. My patches do not remove support for post_init(), they drop the callbacks code. Thus one can reinstate necessary code back. > > > So solve this by splitting the DBI read/writes to .post_init. > > > > Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code") > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > --- > > 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 51fed83484af..da6d79d61397 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) > > @@ -1598,6 +1605,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 > > -- With best wishes Dmitry
On Tue, 21 Jun 2022 at 22:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > cause the system to hang as its using DBI registers in the .init > > and those are only accesible after phy_power_on(). > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > this patch? I don't see the connection. Hi Bjorn, Not really, I can remove it from the description as this only affects the Gen2 port, Gen3 support is dependant on the IPQ6018 support getting merged as it uses the same controller. > > I see that qcom_pcie_host_init() does: > > qcom_pcie_host_init > pcie->cfg->ops->init(pcie) > phy_power_on(pcie->phy) > pcie->cfg->ops->post_init(pcie) > > and that you're moving DBI register accesses from > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > But I also see DBI register accesses in other .init() functions: > > qcom_pcie_init_2_1_0 > qcom_pcie_init_1_0_0 (oddly out of order) > qcom_pcie_init_2_3_2 > qcom_pcie_init_2_4_0 > > Why do these accesses not need to be moved? I assume it's because > pcie->phy is an optional PHY and phy_power_on() does nothing on those > controllers? As far as I could figure out from QCA-s 5.4 kernel, various commits, and QCA-s attempts to solve this already upstream the Gen2 controller in IPQ8074 is a bit special and requires the PHY to be powered on before DBI could be accessed or else the board will hang as it does for me. I can only assume that this is an IPQ8074-specific quirk and other IP-s are not affected like this, so they were not broken. > > Whatever the reason, I think the DBI accesses should be done > consistently in .post_init(). I see that Dmitry's previous patches > removed all those .post_init() functions, but I think the consistency > is worth having. > > Perhaps we could reorder the patches so this patch comes first, moves > the DBI accesses into .post_init(), then Dmitry's patches could be > rebased on top to drop the clock handling? > > > So solve this by splitting the DBI read/writes to .post_init. I am open to anything to get this fixed properly, you are gonna need to point me in the right direction as I am really new to PCI. Regards, Robert > > > > Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code") > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > --- > > 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 51fed83484af..da6d79d61397 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) > > @@ -1598,6 +1605,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 > >
On Tue, Jun 21, 2022 at 11:45:12PM +0300, Dmitry Baryshkov wrote: > On Tue, 21 Jun 2022 at 23:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > > cause the system to hang as its using DBI registers in the .init > > > and those are only accesible after phy_power_on(). > > > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > > this patch? I don't see the connection. > > > > I see that qcom_pcie_host_init() does: > > > > qcom_pcie_host_init > > pcie->cfg->ops->init(pcie) > > phy_power_on(pcie->phy) > > pcie->cfg->ops->post_init(pcie) > > > > and that you're moving DBI register accesses from > > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > > > But I also see DBI register accesses in other .init() functions: > > > > qcom_pcie_init_2_1_0 > > qcom_pcie_init_1_0_0 (oddly out of order) > > qcom_pcie_init_2_3_2 > > qcom_pcie_init_2_4_0 > > > > Why do these accesses not need to be moved? I assume it's because > > pcie->phy is an optional PHY and phy_power_on() does nothing on those > > controllers? > > > > Whatever the reason, I think the DBI accesses should be done > > consistently in .post_init(). I see that Dmitry's previous patches > > removed all those .post_init() functions, but I think the consistency > > is worth having. > > > > Perhaps we could reorder the patches so this patch comes first, moves > > the DBI accesses into .post_init(), then Dmitry's patches could be > > rebased on top to drop the clock handling? > > I don't think there is a need to reorder patches. My patches do not > remove support for post_init(), they drop the callbacks code. Thus one > can reinstate necessary code back. There's not a *need* to reorder them, but I think it would make the patches smaller and more readable because we wouldn't be removing and then re-adding the functions.
On Tue, Jun 21, 2022 at 11:05:17PM +0200, Robert Marko wrote: > On Tue, 21 Jun 2022 at 22:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > > cause the system to hang as its using DBI registers in the .init > > > and those are only accesible after phy_power_on(). > > > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > > this patch? I don't see the connection. > > Not really, I can remove it from the description as this only affects the Gen2 > port, Gen3 support is dependant on the IPQ6018 support getting merged as > it uses the same controller. Great, I think unrelated details confuse things. > > I see that qcom_pcie_host_init() does: > > > > qcom_pcie_host_init > > pcie->cfg->ops->init(pcie) > > phy_power_on(pcie->phy) > > pcie->cfg->ops->post_init(pcie) > > > > and that you're moving DBI register accesses from > > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > > > But I also see DBI register accesses in other .init() functions: > > > > qcom_pcie_init_2_1_0 > > qcom_pcie_init_1_0_0 (oddly out of order) > > qcom_pcie_init_2_3_2 > > qcom_pcie_init_2_4_0 > > > > Why do these accesses not need to be moved? I assume it's because > > pcie->phy is an optional PHY and phy_power_on() does nothing on those > > controllers? > > As far as I could figure out from QCA-s 5.4 kernel, various commits, > and QCA-s attempts to solve this already upstream the Gen2 > controller in IPQ8074 is a bit special and requires the PHY to be > powered on before DBI could be accessed or else the board will hang > as it does for me. > > I can only assume that this is an IPQ8074-specific quirk and other > IP-s are not affected like this, so they were not broken. > > Whatever the reason, I think the DBI accesses should be done > > consistently in .post_init(). I see that Dmitry's previous patches > > removed all those .post_init() functions, but I think the consistency > > is worth having. > > > > Perhaps we could reorder the patches so this patch comes first, moves > > the DBI accesses into .post_init(), then Dmitry's patches could be > > rebased on top to drop the clock handling? > > > > > So solve this by splitting the DBI read/writes to .post_init. > > I am open to anything to get this fixed properly, you are gonna need > to point me in the right direction as I am really new to PCI. Unless there's a reason *not* to move the DBI accesses for other variants, I think we should move them all to .post_init(). Otherwise it's just magic -- there's no indication in the code about why IPQ8074 needs to be different from all the rest. a0fd361db8e5 appeared in v5.11, so presumably IPQ8074 has been broken since then? Are there any problem report URLs or references to the attempts you mentioned above that we could include here? Since folks may want to backport the fix to v5.11, I'd probably do this patch by itself to make the backport easier, then add a second patch to move the DBI accesses for all the other variants. My personal opinion is that you should do this based on v5.19-rc1, and then we can rebase Dmitry's patches on top. That would make all the patches simpler and it would make yours easier to backport. But that's the sort of thing Dmitry, Stanimir, Andy, and Bjorn A. could weigh in on. Bjorn H.
On Tue, Jun 21, 2022 at 03:32:11PM -0500, Bjorn Helgaas wrote: > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > cause the system to hang as its using DBI registers in the .init > > and those are only accesible after phy_power_on(). > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > this patch? I don't see the connection. > > I see that qcom_pcie_host_init() does: > > qcom_pcie_host_init > pcie->cfg->ops->init(pcie) > phy_power_on(pcie->phy) > pcie->cfg->ops->post_init(pcie) > > and that you're moving DBI register accesses from > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > But I also see DBI register accesses in other .init() functions: > > qcom_pcie_init_2_1_0 > qcom_pcie_init_1_0_0 (oddly out of order) > qcom_pcie_init_2_3_2 > qcom_pcie_init_2_4_0 > > Why do these accesses not need to be moved? I assume it's because > pcie->phy is an optional PHY and phy_power_on() does nothing on those > controllers? At least the QMP PHY driver does not implement the PHY power_on op and instead fires everything up already at phy_init(). That may explain the difference in behaviour here. Johan
On 22/06/2022 00:16, Bjorn Helgaas wrote: > On Tue, Jun 21, 2022 at 11:45:12PM +0300, Dmitry Baryshkov wrote: >> On Tue, 21 Jun 2022 at 23:32, Bjorn Helgaas <helgaas@kernel.org> wrote: >>> On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: >>>> IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will >>>> cause the system to hang as its using DBI registers in the .init >>>> and those are only accesible after phy_power_on(). >>> >>> Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to >>> this patch? I don't see the connection. >>> >>> I see that qcom_pcie_host_init() does: >>> >>> qcom_pcie_host_init >>> pcie->cfg->ops->init(pcie) >>> phy_power_on(pcie->phy) >>> pcie->cfg->ops->post_init(pcie) >>> >>> and that you're moving DBI register accesses from >>> qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). >>> >>> But I also see DBI register accesses in other .init() functions: >>> >>> qcom_pcie_init_2_1_0 >>> qcom_pcie_init_1_0_0 (oddly out of order) >>> qcom_pcie_init_2_3_2 >>> qcom_pcie_init_2_4_0 >>> >>> Why do these accesses not need to be moved? I assume it's because >>> pcie->phy is an optional PHY and phy_power_on() does nothing on those >>> controllers? >>> >>> Whatever the reason, I think the DBI accesses should be done >>> consistently in .post_init(). I see that Dmitry's previous patches >>> removed all those .post_init() functions, but I think the consistency >>> is worth having. >>> >>> Perhaps we could reorder the patches so this patch comes first, moves >>> the DBI accesses into .post_init(), then Dmitry's patches could be >>> rebased on top to drop the clock handling? >> >> I don't think there is a need to reorder patches. My patches do not >> remove support for post_init(), they drop the callbacks code. Thus one >> can reinstate necessary code back. > > There's not a *need* to reorder them, but I think it would make the > patches smaller and more readable because we wouldn't be removing and > then re-adding the functions. Ack. I'm fine then with rebasing my patches on top of Robert's patchset. I'll send the next revision after getting this patchset into the form.
On Wed, Jun 22, 2022 at 08:49:20AM +0200, Johan Hovold wrote: > On Tue, Jun 21, 2022 at 03:32:11PM -0500, Bjorn Helgaas wrote: > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > > cause the system to hang as its using DBI registers in the .init > > > and those are only accesible after phy_power_on(). > > But I also see DBI register accesses in other .init() functions: > > > > qcom_pcie_init_2_1_0 > > qcom_pcie_init_1_0_0 (oddly out of order) > > qcom_pcie_init_2_3_2 > > qcom_pcie_init_2_4_0 > > > > Why do these accesses not need to be moved? I assume it's because > > pcie->phy is an optional PHY and phy_power_on() does nothing on those > > controllers? > > At least the QMP PHY driver does not implement the PHY power_on op and > instead fires everything up already at phy_init(). That may explain the > difference in behaviour here. Or maybe not, IPQ8074 appears to be using the same PHY driver. Johan
On Tue, 21 Jun 2022 at 23:43, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 21, 2022 at 11:05:17PM +0200, Robert Marko wrote: > > On Tue, 21 Jun 2022 at 22:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > > > cause the system to hang as its using DBI registers in the .init > > > > and those are only accesible after phy_power_on(). > > > > > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > > > this patch? I don't see the connection. > > > > Not really, I can remove it from the description as this only affects the Gen2 > > port, Gen3 support is dependant on the IPQ6018 support getting merged as > > it uses the same controller. > > Great, I think unrelated details confuse things. > > > > I see that qcom_pcie_host_init() does: > > > > > > qcom_pcie_host_init > > > pcie->cfg->ops->init(pcie) > > > phy_power_on(pcie->phy) > > > pcie->cfg->ops->post_init(pcie) > > > > > > and that you're moving DBI register accesses from > > > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > > > > > But I also see DBI register accesses in other .init() functions: > > > > > > qcom_pcie_init_2_1_0 > > > qcom_pcie_init_1_0_0 (oddly out of order) > > > qcom_pcie_init_2_3_2 > > > qcom_pcie_init_2_4_0 > > > > > > Why do these accesses not need to be moved? I assume it's because > > > pcie->phy is an optional PHY and phy_power_on() does nothing on those > > > controllers? > > > > As far as I could figure out from QCA-s 5.4 kernel, various commits, > > and QCA-s attempts to solve this already upstream the Gen2 > > controller in IPQ8074 is a bit special and requires the PHY to be > > powered on before DBI could be accessed or else the board will hang > > as it does for me. > > > > I can only assume that this is an IPQ8074-specific quirk and other > > IP-s are not affected like this, so they were not broken. > > > > Whatever the reason, I think the DBI accesses should be done > > > consistently in .post_init(). I see that Dmitry's previous patches > > > removed all those .post_init() functions, but I think the consistency > > > is worth having. > > > > > > Perhaps we could reorder the patches so this patch comes first, moves > > > the DBI accesses into .post_init(), then Dmitry's patches could be > > > rebased on top to drop the clock handling? > > > > > > > So solve this by splitting the DBI read/writes to .post_init. > > > > I am open to anything to get this fixed properly, you are gonna need > > to point me in the right direction as I am really new to PCI. > > Unless there's a reason *not* to move the DBI accesses for other > variants, I think we should move them all to .post_init(). Otherwise > it's just magic -- there's no indication in the code about why IPQ8074 > needs to be different from all the rest. Hi Bjorn, I am not sure how to do it properly, especially for the 2.1.0 IP that IPQ8064 uses and that is already filled with tweaks to get it properly working. As far as I can tell, the reason why it works for all of the others is that they dont use a PHY driver at all (2.1.0 in IPQ8064 and 2.4.0 in IPQ4019), 1.1.0 in APQ8084 appears to be unused completely as its compatible is not used in any of the DTS-es. 2.3.2 in MSM8996 and MSM8998 also use QMP, so I am not sure why these work. > > a0fd361db8e5 appeared in v5.11, so presumably IPQ8074 has been broken > since then? Are there any problem report URLs or references to the > attempts you mentioned above that we could include here? It has been broken since then, it worked on 5.10 when I started poking around IPQ8074 and after backporting the 5.11 commits to get the Gen3 port working it stopped working, and I traced that down to hanging after a DBI write. This appears to be QCA-s attempt to always power on the PHY before the init: https://patchwork.kernel.org/project/linux-pci/patch/1596036607-11877-6-git-send-email-sivaprak@codeaurora.org/ > > Since folks may want to backport the fix to v5.11, I'd probably do > this patch by itself to make the backport easier, then add a second > patch to move the DBI accesses for all the other variants. > > My personal opinion is that you should do this based on v5.19-rc1, and > then we can rebase Dmitry's patches on top. That would make all the > patches simpler and it would make yours easier to backport. But > that's the sort of thing Dmitry, Stanimir, Andy, and Bjorn A. could > weigh in on. This patch applies to 5.19 as well, though I will send a v4 with the updated description. Like, I said, I am not sure how to move DBI accesses in other IP-s without breaking them. Regards, Robert > > Bjorn H.
On Wed, Jun 22, 2022 at 08:49:19AM +0200, Johan Hovold wrote: > On Tue, Jun 21, 2022 at 03:32:11PM -0500, Bjorn Helgaas wrote: > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > > cause the system to hang as its using DBI registers in the .init > > > and those are only accesible after phy_power_on(). > > > > Is the fact that IPQ8074 has both a Gen2 and a Gen3 port relevant to > > this patch? I don't see the connection. > > > > I see that qcom_pcie_host_init() does: > > > > qcom_pcie_host_init > > pcie->cfg->ops->init(pcie) > > phy_power_on(pcie->phy) > > pcie->cfg->ops->post_init(pcie) > > > > and that you're moving DBI register accesses from > > qcom_pcie_init_2_3_3() to qcom_pcie_post_init_2_3_3(). > > > > But I also see DBI register accesses in other .init() functions: > > > > qcom_pcie_init_2_1_0 > > qcom_pcie_init_1_0_0 (oddly out of order) > > qcom_pcie_init_2_3_2 > > qcom_pcie_init_2_4_0 > > > > Why do these accesses not need to be moved? I assume it's because > > pcie->phy is an optional PHY and phy_power_on() does nothing on those > > controllers? > > At least the QMP PHY driver does not implement the PHY power_on op and > instead fires everything up already at phy_init(). That may explain the > difference in behaviour here. That was due to a bug in -next which as since been fixed by commit 5bef2838f1a0 ("phy: qcom-qmp: fix PCIe PHY support") which again do all PHY init at phy_power_on() instead of at phy_init(). Note also that before commit cc1e06f033af ("phy: qcom: qmp: Use power_on/off ops for PCIe") everything was done at init. Johan
On Wed, Jun 22, 2022 at 04:23:49PM +0200, Robert Marko wrote: > On Tue, 21 Jun 2022 at 23:43, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Jun 21, 2022 at 11:05:17PM +0200, Robert Marko wrote: > > > On Tue, 21 Jun 2022 at 22:32, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Tue, Jun 21, 2022 at 01:23:30PM +0200, Robert Marko wrote: > > > > > IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will > > > > > cause the system to hang as its using DBI registers in the .init > > > > > and those are only accesible after phy_power_on(). > ... > > Unless there's a reason *not* to move the DBI accesses for other > > variants, I think we should move them all to .post_init(). Otherwise > > it's just magic -- there's no indication in the code about why IPQ8074 > > needs to be different from all the rest. > > I am not sure how to do it properly, especially for the 2.1.0 IP that > IPQ8064 uses > and that is already filled with tweaks to get it properly working. > > As far as I can tell, the reason why it works for all of the others > is that they dont use a PHY driver at all (2.1.0 in IPQ8064 and > 2.4.0 in IPQ4019), 1.1.0 in APQ8084 appears to be unused completely > as its compatible is not used in any of the DTS-es. 2.3.2 in > MSM8996 and MSM8998 also use QMP, so I am not sure why these work. > ... > This patch applies to 5.19 as well, though I will send a v4 with the > updated description. Like, I said, I am not sure how to move DBI > accesses in other IP-s without breaking them. Why would they break? I don't see anything that indicates the DBI accesses are required to be before phy_power_on() or would fail after phy_power_on(). I agree there's always a risk of unintended breakage. It just doesn't seem very likely. Bjorn
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c index 51fed83484af..da6d79d61397 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) @@ -1598,6 +1605,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, };
IPQ8074 has one Gen2 and one Gen3 port, currently the Gen2 port will cause the system to hang as its using DBI registers in the .init and those are only accesible after phy_power_on(). So solve this by splitting the DBI read/writes to .post_init. Fixes: a0fd361db8e5 ("PCI: dwc: Move "dbi", "dbi2", and "addr_space" resource setup into common code") Signed-off-by: Robert Marko <robimarko@gmail.com> --- Changes in v2: * Rebase onto next-20220621 --- drivers/pci/controller/dwc/pcie-qcom.c | 48 +++++++++++++++----------- 1 file changed, 28 insertions(+), 20 deletions(-)