[v2] PCI: qcom: fix IPQ8074 Gen2 support

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(-)

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 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