[PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Robert Marko]

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

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

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Dmitry Baryshkov]

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
>


-- 
With best wishes
Dmitry

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Robert Marko]

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

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Dmitry Baryshkov]

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


-- 
With best wishes
Dmitry

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Bjorn Helgaas]

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
> 

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Dmitry Baryshkov]

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

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Bjorn Helgaas]

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.

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Robert Marko]

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

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Bjorn Helgaas]

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.

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Robert Marko]

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.

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Bjorn Helgaas]

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

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Johan Hovold]

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

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Johan Hovold]

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

Re: [PATCH v2] PCI: qcom: fix IPQ8074 Gen2 support

[Johan Hovold]

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