[PATCH] PCI: qcom-ep: Do not enable resources during probe()

[PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
for drivers requiring refclk from host"), all the hardware register access
(like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
from host.

So there is no need to enable the endpoint resources (like clk, regulators,
PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
helper from probe(). This was added earlier because dw_pcie_ep_init() was
doing DBI access, which is not done now.

While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
EP controller in the case of failure.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom-ep.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 236229f66c80..2319ff2ae9f6 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -846,21 +846,15 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = qcom_pcie_enable_resources(pcie_ep);
-	if (ret) {
-		dev_err(dev, "Failed to enable resources: %d\n", ret);
-		return ret;
-	}
-
 	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
 	if (ret) {
 		dev_err(dev, "Failed to initialize endpoint: %d\n", ret);
-		goto err_disable_resources;
+		return ret;
 	}
 
 	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
 	if (ret)
-		goto err_disable_resources;
+		goto err_ep_deinit;
 
 	name = devm_kasprintf(dev, GFP_KERNEL, "%pOFP", dev->of_node);
 	if (!name) {
@@ -877,8 +871,8 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
 	disable_irq(pcie_ep->global_irq);
 	disable_irq(pcie_ep->perst_irq);
 
-err_disable_resources:
-	qcom_pcie_disable_resources(pcie_ep);
+err_ep_deinit:
+	dw_pcie_ep_deinit(&pcie_ep->pci.ep);
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Dmitry Baryshkov]

On Sat, Jul 27, 2024 at 02:36:04PM GMT, Manivannan Sadhasivam wrote:
> Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> for drivers requiring refclk from host"), all the hardware register access
> (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> from host.
> 
> So there is no need to enable the endpoint resources (like clk, regulators,
> PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> helper from probe(). This was added earlier because dw_pcie_ep_init() was
> doing DBI access, which is not done now.

... moreover his makes PCIe EP fail on some of the platforms as powering
on PHY requires refclk from the RC side, which is not enabled at the
probe time.

Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> EP controller in the case of failure.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)

-- 
With best wishes
Dmitry

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

On Sat, Jul 27, 2024 at 01:32:40PM +0300, Dmitry Baryshkov wrote:
> On Sat, Jul 27, 2024 at 02:36:04PM GMT, Manivannan Sadhasivam wrote:
> > Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> > for drivers requiring refclk from host"), all the hardware register access
> > (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> > in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> > from host.
> > 
> > So there is no need to enable the endpoint resources (like clk, regulators,
> > PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> > helper from probe(). This was added earlier because dw_pcie_ep_init() was
> > doing DBI access, which is not done now.
> 
> ... moreover his makes PCIe EP fail on some of the platforms as powering
> on PHY requires refclk from the RC side, which is not enabled at the
> probe time.
> 

Yeah. I hope Bjorn/Krzysztof could add this to the commit message while
applying.

> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 

Thanks!

- Mani

> > While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> > EP controller in the case of failure.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> -- 
> With best wishes
> Dmitry

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Krzysztof Wilczyński]

Hello,

[...]
> > > So there is no need to enable the endpoint resources (like clk, regulators,
> > > PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> > > helper from probe(). This was added earlier because dw_pcie_ep_init() was
> > > doing DBI access, which is not done now.
> > 
> > ... moreover his makes PCIe EP fail on some of the platforms as powering
> > on PHY requires refclk from the RC side, which is not enabled at the
> > probe time.
> > 
> 
> Yeah. I hope Bjorn/Krzysztof could add this to the commit message while
> applying.

No worries. Added to the commit log.

	Krzysztof

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Krzysztof Wilczyński]

Hello,

> Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> for drivers requiring refclk from host"), all the hardware register access
> (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> from host.
> 
> So there is no need to enable the endpoint resources (like clk, regulators,
> PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> helper from probe(). This was added earlier because dw_pcie_ep_init() was
> doing DBI access, which is not done now.
> 
> While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> EP controller in the case of failure.

Applied to controller/qcom, thank you!

[1/1] PCI: qcom-ep: Do not enable resources during probe()
      https://git.kernel.org/pci/pci/c/cd0b3e13ec30

	Krzysztof

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Bjorn Helgaas]

On Wed, Aug 14, 2024 at 05:25:47AM +0900, Krzysztof Wilczyński wrote:
> > Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> > for drivers requiring refclk from host"), all the hardware register access
> > (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> > in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> > from host.
> > 
> > So there is no need to enable the endpoint resources (like clk, regulators,
> > PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> > helper from probe(). This was added earlier because dw_pcie_ep_init() was
> > doing DBI access, which is not done now.
> > 
> > While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> > EP controller in the case of failure.
> 
> Applied to controller/qcom, thank you!
> 
> [1/1] PCI: qcom-ep: Do not enable resources during probe()
>       https://git.kernel.org/pci/pci/c/cd0b3e13ec30

I think we do need this, but I dropped it for now pending a commit log
that says "we're fixing a crash" and explains how.

The current log says "869bc5253406 moved hardware register access like
DBI to dw_pcie_ep_init_registers()", but 869bc5253406 actually moved a
bunch of register accesses from dw_pcie_ep_init() to
dw_pcie_ep_init_complete(), and a subsequent patch renamed
dw_pcie_ep_init_complete() to dw_pcie_ep_init_registers().  I did
eventually figure out the rename, but it took a while to make that
leap.

It also says dw_pcie_ep_init_registers() is called only from
qcom_pcie_perst_deassert(), but obviously all drivers call it.  I
think what you meant is that on qcom and tegra194,
dw_pcie_ep_init_registers() isn't called from .probe(); it's called
later because they require refclk to access the registers, so qcom and
tegra194 call it after PERST# is deasserted, because then refclk is
available.

Trying to understand the 869bc5253406 reference: I guess the
point is that the dw_pcie_ep_init_registers() work depends on
qcom_pcie_enable_resources(), and before 869bc5253406, that work was
done by qcom_pcie_ep_probe() calling dw_pcie_ep_init(), so it had to
call qcom_pcie_enable_resources() first.

But after 869bc5253406, dw_pcie_ep_init_registers() is done in
qcom_pcie_perst_deassert(), which already calls
qcom_pcie_enable_resources().  So qcom_pcie_ep_probe() no longer needs
to call qcom_pcie_enable_resources().

As far as the *crash*, phy_power_on() has been called from
qcom_pcie_ep_probe() since the very beginning in f55fee56a631 ("PCI:
qcom-ep: Add Qualcomm PCIe Endpoint controller driver").  But
apparently on some new platforms phy_power_on() depends on refclk and
(I assume) it causes a crash when done from qcom_pcie_ep_probe().

So I would think the commit log should look something like this:

  PCI: qcom-ep: Postpone PHY power-on until refclk available

  qcom_pcie_enable_resources() is called by qcom_pcie_ep_probe() and
  powers on PHYs.  On new platforms like X, Y, Z, this depends on
  refclk from the RC, which may not be available at the time of
  qcom_pcie_ep_probe(), so this causes a crash in the qcom-ep driver.

  qcom_pcie_enable_resources() is already called by
  qcom_pcie_perst_deassert() when PERST# is deasserted, and refclk is
  available at that time.

  Remove the unnecessary call from qcom_pcie_ep_probe() to prevent the
  crash on X, Y, Z.

Although I do have the question of what happens if the RC deasserts
PERST# before qcom-ep is loaded.  We probably don't execute
qcom_pcie_perst_deassert() in that case, so how does the init happen?

Bjorn

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 14, 2024 at 05:25:47AM +0900, Krzysztof Wilczyński wrote:
> > > Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> > > for drivers requiring refclk from host"), all the hardware register access
> > > (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> > > in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> > > from host.
> > > 
> > > So there is no need to enable the endpoint resources (like clk, regulators,
> > > PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> > > helper from probe(). This was added earlier because dw_pcie_ep_init() was
> > > doing DBI access, which is not done now.
> > > 
> > > While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> > > EP controller in the case of failure.
> > 
> > Applied to controller/qcom, thank you!
> > 
> > [1/1] PCI: qcom-ep: Do not enable resources during probe()
> >       https://git.kernel.org/pci/pci/c/cd0b3e13ec30
> 
> I think we do need this, but I dropped it for now pending a commit log
> that says "we're fixing a crash" and explains how.
> 
> The current log says "869bc5253406 moved hardware register access like
> DBI to dw_pcie_ep_init_registers()", but 869bc5253406 actually moved a
> bunch of register accesses from dw_pcie_ep_init() to
> dw_pcie_ep_init_complete(), and a subsequent patch renamed
> dw_pcie_ep_init_complete() to dw_pcie_ep_init_registers().  I did
> eventually figure out the rename, but it took a while to make that
> leap.
> 

Ah, sorry about that. I somehow managed to merge those two commits in my mind.

> It also says dw_pcie_ep_init_registers() is called only from
> qcom_pcie_perst_deassert(), but obviously all drivers call it.  I
> think what you meant is that on qcom and tegra194,

Yeah, the context is only applicable to the respective SoCs.

> dw_pcie_ep_init_registers() isn't called from .probe(); it's called
> later because they require refclk to access the registers, so qcom and
> tegra194 call it after PERST# is deasserted, because then refclk is
> available.
> 
> Trying to understand the 869bc5253406 reference: I guess the
> point is that the dw_pcie_ep_init_registers() work depends on
> qcom_pcie_enable_resources(), and before 869bc5253406, that work was
> done by qcom_pcie_ep_probe() calling dw_pcie_ep_init(), so it had to
> call qcom_pcie_enable_resources() first.
> 
> But after 869bc5253406, dw_pcie_ep_init_registers() is done in
> qcom_pcie_perst_deassert(), which already calls
> qcom_pcie_enable_resources().  So qcom_pcie_ep_probe() no longer needs
> to call qcom_pcie_enable_resources().
> 

Right.

> As far as the *crash*, phy_power_on() has been called from
> qcom_pcie_ep_probe() since the very beginning in f55fee56a631 ("PCI:
> qcom-ep: Add Qualcomm PCIe Endpoint controller driver").  But
> apparently on some new platforms phy_power_on() depends on refclk and
> (I assume) it causes a crash when done from qcom_pcie_ep_probe().
> 

Yeah. The actual intention of the patch was to get rid of the
qcom_pcie_enable_resources() from probe() as it is now not needed. But later
Dmitry pointed out that qcom_pcie_enable_resources() is also causing a crash on
his platform because phy_power_on() depends on refclk from host. So he had to
manually boot the host first and then endpoint to avoid the crash (without this
patch).

Then I thought that this is an RC candidate as it fixes a hard crash and asked
to merge the patch ammending the commit message with Dmitry's info.

But now I understood that I should've reworded the commit message to make it
look like an RC material.

> So I would think the commit log should look something like this:
> 
>   PCI: qcom-ep: Postpone PHY power-on until refclk available
> 
>   qcom_pcie_enable_resources() is called by qcom_pcie_ep_probe() and
>   powers on PHYs.  On new platforms like X, Y, Z, this depends on
>   refclk from the RC, which may not be available at the time of
>   qcom_pcie_ep_probe(), so this causes a crash in the qcom-ep driver.
> 
>   qcom_pcie_enable_resources() is already called by
>   qcom_pcie_perst_deassert() when PERST# is deasserted, and refclk is
>   available at that time.
> 
>   Remove the unnecessary call from qcom_pcie_ep_probe() to prevent the
>   crash on X, Y, Z.
> 

Looks good to me. I'll post v2 incorporating this.

> Although I do have the question of what happens if the RC deasserts
> PERST# before qcom-ep is loaded.  We probably don't execute
> qcom_pcie_perst_deassert() in that case, so how does the init happen?
> 

PERST# is a level trigger signal. So even if the host has asserted it before EP
booted, the level will stay low and ep will detect it while booting.

If it is an edge trigger signal, then ep wouldn't be able to catch it as you
suspected.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Bjorn Helgaas]

On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> ...

> > Although I do have the question of what happens if the RC deasserts
> > PERST# before qcom-ep is loaded.  We probably don't execute
> > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> 
> PERST# is a level trigger signal. So even if the host has asserted
> it before EP booted, the level will stay low and ep will detect it
> while booting.

The PERST# signal itself is definitely level oriented.

I'm still skeptical about the *interrupt* from the PCIe controller
being level-triggered, as I mentioned here:
https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas

tegra194 is also dwc-based and has a similar PERST# interrupt but it's
edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think is a
cleaner implementation.  Then you don't have to remember the current
state, switch between high and low trigger, worry about races and
missing a pulse, etc.

> If it is an edge trigger signal, then ep wouldn't be able to catch
> it as you suspected.

Bjorn

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 10:16:58AM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> > ...
> 
> > > Although I do have the question of what happens if the RC deasserts
> > > PERST# before qcom-ep is loaded.  We probably don't execute
> > > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> > 
> > PERST# is a level trigger signal. So even if the host has asserted
> > it before EP booted, the level will stay low and ep will detect it
> > while booting.
> 
> The PERST# signal itself is definitely level oriented.
> 
> I'm still skeptical about the *interrupt* from the PCIe controller
> being level-triggered, as I mentioned here:
> https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas
> 

Sorry, that comment got buried into my inbox. So didn't get a chance to respond.

> tegra194 is also dwc-based and has a similar PERST# interrupt but it's
> edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think is a
> cleaner implementation.  Then you don't have to remember the current
> state, switch between high and low trigger, worry about races and
> missing a pulse, etc.
> 

I did try to mimic what tegra194 did when I wrote the qcom-ep driver, but it
didn't work. If we use the level triggered interrupt as edge, the interrupt will
be missed if we do not listen at the right time (when PERST# goes from high to
low and vice versa).

I don't know how tegra194 interrupt controller is wired up, but IIUC they will
need to boot the endpoint first and then host to catch the PERST# interrupt.
Otherwise, the endpoint will never see the interrupt until host toggles it
again.

But there is no point in forcing this ordering and that was the reason why I
went with the level triggered approach.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Bjorn Helgaas]

On Thu, Aug 22, 2024 at 09:10:25PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 22, 2024 at 10:16:58AM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> > > ...
> > 
> > > > Although I do have the question of what happens if the RC deasserts
> > > > PERST# before qcom-ep is loaded.  We probably don't execute
> > > > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> > > 
> > > PERST# is a level trigger signal. So even if the host has asserted
> > > it before EP booted, the level will stay low and ep will detect it
> > > while booting.
> > 
> > The PERST# signal itself is definitely level oriented.
> > 
> > I'm still skeptical about the *interrupt* from the PCIe controller
> > being level-triggered, as I mentioned here:
> > https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas
> 
> Sorry, that comment got buried into my inbox. So didn't get a chance
> to respond.
> 
> > tegra194 is also dwc-based and has a similar PERST# interrupt but
> > it's edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think
> > is a cleaner implementation.  Then you don't have to remember the
> > current state, switch between high and low trigger, worry about
> > races and missing a pulse, etc.
> 
> I did try to mimic what tegra194 did when I wrote the qcom-ep
> driver, but it didn't work. If we use the level triggered interrupt
> as edge, the interrupt will be missed if we do not listen at the
> right time (when PERST# goes from high to low and vice versa).
> 
> I don't know how tegra194 interrupt controller is wired up, but IIUC
> they will need to boot the endpoint first and then host to catch the
> PERST# interrupt.  Otherwise, the endpoint will never see the
> interrupt until host toggles it again.

Having to control the boot ordering of endpoint and host is definitely
problematic.

What is the nature of the crash when we try to enable the PHY when
Refclk is not available?  The endpoint has no control over when the
host asserts/deasserts PERST#.  If PERST# happens to be asserted while
the endpoint is enabling the PHY, and this causes some kind of crash
that the endpoint driver can't easily recover from, that's a serious
robustness problem.

> But there is no point in forcing this ordering and that was the
> reason why I went with the level triggered approach.

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

On Thu, Aug 22, 2024 at 12:31:33PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 22, 2024 at 09:10:25PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 22, 2024 at 10:16:58AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> > > > On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> > > > ...
> > > 
> > > > > Although I do have the question of what happens if the RC deasserts
> > > > > PERST# before qcom-ep is loaded.  We probably don't execute
> > > > > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> > > > 
> > > > PERST# is a level trigger signal. So even if the host has asserted
> > > > it before EP booted, the level will stay low and ep will detect it
> > > > while booting.
> > > 
> > > The PERST# signal itself is definitely level oriented.
> > > 
> > > I'm still skeptical about the *interrupt* from the PCIe controller
> > > being level-triggered, as I mentioned here:
> > > https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas
> > 
> > Sorry, that comment got buried into my inbox. So didn't get a chance
> > to respond.
> > 
> > > tegra194 is also dwc-based and has a similar PERST# interrupt but
> > > it's edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think
> > > is a cleaner implementation.  Then you don't have to remember the
> > > current state, switch between high and low trigger, worry about
> > > races and missing a pulse, etc.
> > 
> > I did try to mimic what tegra194 did when I wrote the qcom-ep
> > driver, but it didn't work. If we use the level triggered interrupt
> > as edge, the interrupt will be missed if we do not listen at the
> > right time (when PERST# goes from high to low and vice versa).
> > 
> > I don't know how tegra194 interrupt controller is wired up, but IIUC
> > they will need to boot the endpoint first and then host to catch the
> > PERST# interrupt.  Otherwise, the endpoint will never see the
> > interrupt until host toggles it again.
> 
> Having to control the boot ordering of endpoint and host is definitely
> problematic.
> 
> What is the nature of the crash when we try to enable the PHY when
> Refclk is not available?  The endpoint has no control over when the
> host asserts/deasserts PERST#.  If PERST# happens to be asserted while
> the endpoint is enabling the PHY, and this causes some kind of crash
> that the endpoint driver can't easily recover from, that's a serious
> robustness problem.
> 

The whole endpoint SoC crashes if the refclk is not available during
phy_power_on() as the PHY driver tries to access some register on Dmitry's
platform (I did not see this crash on SM8450 SoC though).

If we keep the enable_resources() during probe() then the race condition you
observed above could apply. So removing that from probe() will also make the
race condition go away,

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Bjorn Helgaas]

On Fri, Aug 23, 2024 at 10:11:33AM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 22, 2024 at 12:31:33PM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 22, 2024 at 09:10:25PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Aug 22, 2024 at 10:16:58AM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> > > > > ...
> > > > 
> > > > > > Although I do have the question of what happens if the RC deasserts
> > > > > > PERST# before qcom-ep is loaded.  We probably don't execute
> > > > > > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> > > > > 
> > > > > PERST# is a level trigger signal. So even if the host has asserted
> > > > > it before EP booted, the level will stay low and ep will detect it
> > > > > while booting.
> > > > 
> > > > The PERST# signal itself is definitely level oriented.
> > > > 
> > > > I'm still skeptical about the *interrupt* from the PCIe controller
> > > > being level-triggered, as I mentioned here:
> > > > https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas
> > > 
> > > Sorry, that comment got buried into my inbox. So didn't get a chance
> > > to respond.
> > > 
> > > > tegra194 is also dwc-based and has a similar PERST# interrupt but
> > > > it's edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think
> > > > is a cleaner implementation.  Then you don't have to remember the
> > > > current state, switch between high and low trigger, worry about
> > > > races and missing a pulse, etc.
> > > 
> > > I did try to mimic what tegra194 did when I wrote the qcom-ep
> > > driver, but it didn't work. If we use the level triggered interrupt
> > > as edge, the interrupt will be missed if we do not listen at the
> > > right time (when PERST# goes from high to low and vice versa).
> > > 
> > > I don't know how tegra194 interrupt controller is wired up, but IIUC
> > > they will need to boot the endpoint first and then host to catch the
> > > PERST# interrupt.  Otherwise, the endpoint will never see the
> > > interrupt until host toggles it again.
> > 
> > Having to control the boot ordering of endpoint and host is definitely
> > problematic.
> > 
> > What is the nature of the crash when we try to enable the PHY when
> > Refclk is not available?  The endpoint has no control over when the
> > host asserts/deasserts PERST#.  If PERST# happens to be asserted while
> > the endpoint is enabling the PHY, and this causes some kind of crash
> > that the endpoint driver can't easily recover from, that's a serious
> > robustness problem.
> 
> The whole endpoint SoC crashes if the refclk is not available during
> phy_power_on() as the PHY driver tries to access some register on Dmitry's
> platform (I did not see this crash on SM8450 SoC though).
> 
> If we keep the enable_resources() during probe() then the race condition you
> observed above could apply. So removing that from probe() will also make the
> race condition go away,

Example:

  1) host deasserts PERST#
  2) qcom-ep handles PERST# IRQ
  3) qcom_pcie_ep_perst_irq_thread() calls qcom_pcie_perst_deassert()
  4) host asserts PERST#, Refclk no longer valid
  5) qcom_pcie_perst_deassert() calls qcom_pcie_enable_resources()
  6) qcom_pcie_enable_resources() enables PHY

I don't see what prevents the PERST# assertion at 4.  It sounds like
the endpoint SoC crashes at 6.

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

On Fri, Aug 23, 2024 at 05:04:36PM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 23, 2024 at 10:11:33AM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 22, 2024 at 12:31:33PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Aug 22, 2024 at 09:10:25PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Aug 22, 2024 at 10:16:58AM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> > > > > > ...
> > > > > 
> > > > > > > Although I do have the question of what happens if the RC deasserts
> > > > > > > PERST# before qcom-ep is loaded.  We probably don't execute
> > > > > > > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> > > > > > 
> > > > > > PERST# is a level trigger signal. So even if the host has asserted
> > > > > > it before EP booted, the level will stay low and ep will detect it
> > > > > > while booting.
> > > > > 
> > > > > The PERST# signal itself is definitely level oriented.
> > > > > 
> > > > > I'm still skeptical about the *interrupt* from the PCIe controller
> > > > > being level-triggered, as I mentioned here:
> > > > > https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas
> > > > 
> > > > Sorry, that comment got buried into my inbox. So didn't get a chance
> > > > to respond.
> > > > 
> > > > > tegra194 is also dwc-based and has a similar PERST# interrupt but
> > > > > it's edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think
> > > > > is a cleaner implementation.  Then you don't have to remember the
> > > > > current state, switch between high and low trigger, worry about
> > > > > races and missing a pulse, etc.
> > > > 
> > > > I did try to mimic what tegra194 did when I wrote the qcom-ep
> > > > driver, but it didn't work. If we use the level triggered interrupt
> > > > as edge, the interrupt will be missed if we do not listen at the
> > > > right time (when PERST# goes from high to low and vice versa).
> > > > 
> > > > I don't know how tegra194 interrupt controller is wired up, but IIUC
> > > > they will need to boot the endpoint first and then host to catch the
> > > > PERST# interrupt.  Otherwise, the endpoint will never see the
> > > > interrupt until host toggles it again.
> > > 
> > > Having to control the boot ordering of endpoint and host is definitely
> > > problematic.
> > > 
> > > What is the nature of the crash when we try to enable the PHY when
> > > Refclk is not available?  The endpoint has no control over when the
> > > host asserts/deasserts PERST#.  If PERST# happens to be asserted while
> > > the endpoint is enabling the PHY, and this causes some kind of crash
> > > that the endpoint driver can't easily recover from, that's a serious
> > > robustness problem.
> > 
> > The whole endpoint SoC crashes if the refclk is not available during
> > phy_power_on() as the PHY driver tries to access some register on Dmitry's
> > platform (I did not see this crash on SM8450 SoC though).
> > 
> > If we keep the enable_resources() during probe() then the race condition you
> > observed above could apply. So removing that from probe() will also make the
> > race condition go away,
> 
> Example:
> 
>   1) host deasserts PERST#
>   2) qcom-ep handles PERST# IRQ
>   3) qcom_pcie_ep_perst_irq_thread() calls qcom_pcie_perst_deassert()
>   4) host asserts PERST#, Refclk no longer valid
>   5) qcom_pcie_perst_deassert() calls qcom_pcie_enable_resources()
>   6) qcom_pcie_enable_resources() enables PHY
> 
> I don't see what prevents the PERST# assertion at 4.  It sounds like
> the endpoint SoC crashes at 6.

IDK why host would quickly assert the PERST# after deasserting during probe()
unless someone intentionally does that from host side.

If that happens then there is a possibility of the endpoint SoC crash, but I'm
not sure how we can avoid that.

But what this patch fixes is a crash occuring in a sane scenario:

1) Endpoint boots first (no refclk from host)
2) Probe() calls qcom_pcie_enable_resources() --> Crash

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Bjorn Helgaas]

On Sat, Aug 24, 2024 at 07:49:46AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Aug 23, 2024 at 05:04:36PM -0500, Bjorn Helgaas wrote:
> > On Fri, Aug 23, 2024 at 10:11:33AM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Aug 22, 2024 at 12:31:33PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Aug 22, 2024 at 09:10:25PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Thu, Aug 22, 2024 at 10:16:58AM -0500, Bjorn Helgaas wrote:
> > > > > > On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> > > > > > > On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> > > > > > > ...
> > > > > > 
> > > > > > > > Although I do have the question of what happens if the RC deasserts
> > > > > > > > PERST# before qcom-ep is loaded.  We probably don't execute
> > > > > > > > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> > > > > > > 
> > > > > > > PERST# is a level trigger signal. So even if the host has asserted
> > > > > > > it before EP booted, the level will stay low and ep will detect it
> > > > > > > while booting.
> > > > > > 
> > > > > > The PERST# signal itself is definitely level oriented.
> > > > > > 
> > > > > > I'm still skeptical about the *interrupt* from the PCIe controller
> > > > > > being level-triggered, as I mentioned here:
> > > > > > https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas
> > > > > 
> > > > > Sorry, that comment got buried into my inbox. So didn't get a chance
> > > > > to respond.
> > > > > 
> > > > > > tegra194 is also dwc-based and has a similar PERST# interrupt but
> > > > > > it's edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think
> > > > > > is a cleaner implementation.  Then you don't have to remember the
> > > > > > current state, switch between high and low trigger, worry about
> > > > > > races and missing a pulse, etc.
> > > > > 
> > > > > I did try to mimic what tegra194 did when I wrote the qcom-ep
> > > > > driver, but it didn't work. If we use the level triggered interrupt
> > > > > as edge, the interrupt will be missed if we do not listen at the
> > > > > right time (when PERST# goes from high to low and vice versa).
> > > > > 
> > > > > I don't know how tegra194 interrupt controller is wired up, but IIUC
> > > > > they will need to boot the endpoint first and then host to catch the
> > > > > PERST# interrupt.  Otherwise, the endpoint will never see the
> > > > > interrupt until host toggles it again.
> > > > 
> > > > Having to control the boot ordering of endpoint and host is definitely
> > > > problematic.
> > > > 
> > > > What is the nature of the crash when we try to enable the PHY when
> > > > Refclk is not available?  The endpoint has no control over when the
> > > > host asserts/deasserts PERST#.  If PERST# happens to be asserted while
> > > > the endpoint is enabling the PHY, and this causes some kind of crash
> > > > that the endpoint driver can't easily recover from, that's a serious
> > > > robustness problem.
> > > 
> > > The whole endpoint SoC crashes if the refclk is not available during
> > > phy_power_on() as the PHY driver tries to access some register on Dmitry's
> > > platform (I did not see this crash on SM8450 SoC though).

I don't think the nature of this crash has been explained, so I don't
know whether it's a recoverable situation or not.

> > > If we keep the enable_resources() during probe() then the race
> > > condition you observed above could apply. So removing that from
> > > probe() will also make the race condition go away,
> > 
> > Example:
> > 
> >   1) host deasserts PERST#
> >   2) qcom-ep handles PERST# IRQ
> >   3) qcom_pcie_ep_perst_irq_thread() calls qcom_pcie_perst_deassert()
> >   4) host asserts PERST#, Refclk no longer valid
> >   5) qcom_pcie_perst_deassert() calls qcom_pcie_enable_resources()
> >   6) qcom_pcie_enable_resources() enables PHY
> > 
> > I don't see what prevents the PERST# assertion at 4.  It sounds like
> > the endpoint SoC crashes at 6.
> 
> IDK why host would quickly assert the PERST# after deasserting
> during probe() unless someone intentionally does that from host
> side.

I think the host is allowed to assert PERST# at any arbitrary time, so
an endpoint should be able to handle it no matter when it happens.

> If that happens then there is a possibility of the endpoint SoC
> crash, but I'm not sure how we can avoid that.
> 
> But what this patch fixes is a crash occuring in a sane scenario:
> 
> 1) Endpoint boots first (no refclk from host)
> 2) Probe() calls qcom_pcie_enable_resources() --> Crash

I agree with this, although I think it's more of a band-aid than a
complete solution.  I don't have access to any SoC or PCIe controller
docs, so maybe this is a hardware design problem and this is the best
we can do.

Bjorn

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

On Sat, Aug 24, 2024 at 11:12:34AM -0500, Bjorn Helgaas wrote:
> On Sat, Aug 24, 2024 at 07:49:46AM +0530, Manivannan Sadhasivam wrote:
> > On Fri, Aug 23, 2024 at 05:04:36PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Aug 23, 2024 at 10:11:33AM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Aug 22, 2024 at 12:31:33PM -0500, Bjorn Helgaas wrote:
> > > > > On Thu, Aug 22, 2024 at 09:10:25PM +0530, Manivannan Sadhasivam wrote:
> > > > > > On Thu, Aug 22, 2024 at 10:16:58AM -0500, Bjorn Helgaas wrote:
> > > > > > > On Thu, Aug 22, 2024 at 12:18:23PM +0530, Manivannan Sadhasivam wrote:
> > > > > > > > On Wed, Aug 21, 2024 at 05:56:18PM -0500, Bjorn Helgaas wrote:
> > > > > > > > ...
> > > > > > > 
> > > > > > > > > Although I do have the question of what happens if the RC deasserts
> > > > > > > > > PERST# before qcom-ep is loaded.  We probably don't execute
> > > > > > > > > qcom_pcie_perst_deassert() in that case, so how does the init happen?
> > > > > > > > 
> > > > > > > > PERST# is a level trigger signal. So even if the host has asserted
> > > > > > > > it before EP booted, the level will stay low and ep will detect it
> > > > > > > > while booting.
> > > > > > > 
> > > > > > > The PERST# signal itself is definitely level oriented.
> > > > > > > 
> > > > > > > I'm still skeptical about the *interrupt* from the PCIe controller
> > > > > > > being level-triggered, as I mentioned here:
> > > > > > > https://lore.kernel.org/r/20240815224735.GA57931@bhelgaas
> > > > > > 
> > > > > > Sorry, that comment got buried into my inbox. So didn't get a chance
> > > > > > to respond.
> > > > > > 
> > > > > > > tegra194 is also dwc-based and has a similar PERST# interrupt but
> > > > > > > it's edge-triggered (tegra_pcie_ep_pex_rst_irq()), which I think
> > > > > > > is a cleaner implementation.  Then you don't have to remember the
> > > > > > > current state, switch between high and low trigger, worry about
> > > > > > > races and missing a pulse, etc.
> > > > > > 
> > > > > > I did try to mimic what tegra194 did when I wrote the qcom-ep
> > > > > > driver, but it didn't work. If we use the level triggered interrupt
> > > > > > as edge, the interrupt will be missed if we do not listen at the
> > > > > > right time (when PERST# goes from high to low and vice versa).
> > > > > > 
> > > > > > I don't know how tegra194 interrupt controller is wired up, but IIUC
> > > > > > they will need to boot the endpoint first and then host to catch the
> > > > > > PERST# interrupt.  Otherwise, the endpoint will never see the
> > > > > > interrupt until host toggles it again.
> > > > > 
> > > > > Having to control the boot ordering of endpoint and host is definitely
> > > > > problematic.
> > > > > 
> > > > > What is the nature of the crash when we try to enable the PHY when
> > > > > Refclk is not available?  The endpoint has no control over when the
> > > > > host asserts/deasserts PERST#.  If PERST# happens to be asserted while
> > > > > the endpoint is enabling the PHY, and this causes some kind of crash
> > > > > that the endpoint driver can't easily recover from, that's a serious
> > > > > robustness problem.
> > > > 
> > > > The whole endpoint SoC crashes if the refclk is not available during
> > > > phy_power_on() as the PHY driver tries to access some register on Dmitry's
> > > > platform (I did not see this crash on SM8450 SoC though).
> 
> I don't think the nature of this crash has been explained, so I don't
> know whether it's a recoverable situation or not.
> 

I will add this info in the commit message.

> > > > If we keep the enable_resources() during probe() then the race
> > > > condition you observed above could apply. So removing that from
> > > > probe() will also make the race condition go away,
> > > 
> > > Example:
> > > 
> > >   1) host deasserts PERST#
> > >   2) qcom-ep handles PERST# IRQ
> > >   3) qcom_pcie_ep_perst_irq_thread() calls qcom_pcie_perst_deassert()
> > >   4) host asserts PERST#, Refclk no longer valid
> > >   5) qcom_pcie_perst_deassert() calls qcom_pcie_enable_resources()
> > >   6) qcom_pcie_enable_resources() enables PHY
> > > 
> > > I don't see what prevents the PERST# assertion at 4.  It sounds like
> > > the endpoint SoC crashes at 6.
> > 
> > IDK why host would quickly assert the PERST# after deasserting
> > during probe() unless someone intentionally does that from host
> > side.
> 
> I think the host is allowed to assert PERST# at any arbitrary time, so
> an endpoint should be able to handle it no matter when it happens.
> 
> > If that happens then there is a possibility of the endpoint SoC
> > crash, but I'm not sure how we can avoid that.
> > 
> > But what this patch fixes is a crash occuring in a sane scenario:
> > 
> > 1) Endpoint boots first (no refclk from host)
> > 2) Probe() calls qcom_pcie_enable_resources() --> Crash
> 
> I agree with this, although I think it's more of a band-aid than a
> complete solution.  I don't have access to any SoC or PCIe controller
> docs, so maybe this is a hardware design problem and this is the best
> we can do.
> 

I agree. But AFAIK there is no way endpoint can avoid this crash unless it
generates its own clock. I did some investigation on the SRIS support and able
to get it work in my local branch.

I will try to upstream that feature for the currerntly supported Qcom SoCs in
endpoint mode. But Qcom told me that non-SRIS mode is also required by some
customers, so unfortunately we cannot make it as the default operating mode.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Krzysztof Wilczyński]

Hello,

[...]
> Applied to controller/qcom, thank you!

Based on the conversation here, I removed this patch from the branch.

	Krzysztof

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Bjorn Helgaas]

On Sat, Jul 27, 2024 at 02:36:04PM +0530, Manivannan Sadhasivam wrote:
> Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> for drivers requiring refclk from host"), all the hardware register access
> (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> from host.
> 
> So there is no need to enable the endpoint resources (like clk, regulators,
> PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> helper from probe(). This was added earlier because dw_pcie_ep_init() was
> doing DBI access, which is not done now.
> 
> While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> EP controller in the case of failure.

Is this v6.11 material?  If so, we need a little more justification
than "no need to enable".

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom-ep.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 236229f66c80..2319ff2ae9f6 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -846,21 +846,15 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = qcom_pcie_enable_resources(pcie_ep);
> -	if (ret) {
> -		dev_err(dev, "Failed to enable resources: %d\n", ret);
> -		return ret;
> -	}
> -
>  	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
>  	if (ret) {
>  		dev_err(dev, "Failed to initialize endpoint: %d\n", ret);
> -		goto err_disable_resources;
> +		return ret;
>  	}
>  
>  	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
>  	if (ret)
> -		goto err_disable_resources;
> +		goto err_ep_deinit;
>  
>  	name = devm_kasprintf(dev, GFP_KERNEL, "%pOFP", dev->of_node);
>  	if (!name) {
> @@ -877,8 +871,8 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>  	disable_irq(pcie_ep->global_irq);
>  	disable_irq(pcie_ep->perst_irq);
>  
> -err_disable_resources:
> -	qcom_pcie_disable_resources(pcie_ep);
> +err_ep_deinit:
> +	dw_pcie_ep_deinit(&pcie_ep->pci.ep);
>  
>  	return ret;
>  }
> -- 
> 2.25.1
> 

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 01:15:57PM -0500, Bjorn Helgaas wrote:
> On Sat, Jul 27, 2024 at 02:36:04PM +0530, Manivannan Sadhasivam wrote:
> > Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> > for drivers requiring refclk from host"), all the hardware register access
> > (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> > in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> > from host.
> > 
> > So there is no need to enable the endpoint resources (like clk, regulators,
> > PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> > helper from probe(). This was added earlier because dw_pcie_ep_init() was
> > doing DBI access, which is not done now.
> > 
> > While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> > EP controller in the case of failure.
> 
> Is this v6.11 material?  If so, we need a little more justification
> than "no need to enable".
> 

That's why I asked to merge the comment from Dmitry:

"...moreover his makes PCIe EP fail on some of the platforms as powering on PHY
requires refclk from the RC side, which is not enabled at the probe time."

And Krzysztof did that:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/qcom&id=cd0b3e13ec309dcbe1efb66b1969fc72088b791d

- Mani

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom-ep.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > index 236229f66c80..2319ff2ae9f6 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> > @@ -846,21 +846,15 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = qcom_pcie_enable_resources(pcie_ep);
> > -	if (ret) {
> > -		dev_err(dev, "Failed to enable resources: %d\n", ret);
> > -		return ret;
> > -	}
> > -
> >  	ret = dw_pcie_ep_init(&pcie_ep->pci.ep);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to initialize endpoint: %d\n", ret);
> > -		goto err_disable_resources;
> > +		return ret;
> >  	}
> >  
> >  	ret = qcom_pcie_ep_enable_irq_resources(pdev, pcie_ep);
> >  	if (ret)
> > -		goto err_disable_resources;
> > +		goto err_ep_deinit;
> >  
> >  	name = devm_kasprintf(dev, GFP_KERNEL, "%pOFP", dev->of_node);
> >  	if (!name) {
> > @@ -877,8 +871,8 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
> >  	disable_irq(pcie_ep->global_irq);
> >  	disable_irq(pcie_ep->perst_irq);
> >  
> > -err_disable_resources:
> > -	qcom_pcie_disable_resources(pcie_ep);
> > +err_ep_deinit:
> > +	dw_pcie_ep_deinit(&pcie_ep->pci.ep);
> >  
> >  	return ret;
> >  }
> > -- 
> > 2.25.1
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

[Bjorn Helgaas]

On Fri, Aug 16, 2024 at 11:07:57AM +0530, Manivannan Sadhasivam wrote:
> On Thu, Aug 15, 2024 at 01:15:57PM -0500, Bjorn Helgaas wrote:
> > On Sat, Jul 27, 2024 at 02:36:04PM +0530, Manivannan Sadhasivam wrote:
> > > Starting from commit 869bc5253406 ("PCI: dwc: ep: Fix DBI access failure
> > > for drivers requiring refclk from host"), all the hardware register access
> > > (like DBI) were moved to dw_pcie_ep_init_registers() which gets called only
> > > in qcom_pcie_perst_deassert() i.e., only after the endpoint received refclk
> > > from host.
> > > 
> > > So there is no need to enable the endpoint resources (like clk, regulators,
> > > PHY) during probe(). Hence, remove the call to qcom_pcie_enable_resources()
> > > helper from probe(). This was added earlier because dw_pcie_ep_init() was
> > > doing DBI access, which is not done now.
> > > 
> > > While at it, let's also call dw_pcie_ep_deinit() in err path to deinit the
> > > EP controller in the case of failure.
> > 
> > Is this v6.11 material?  If so, we need a little more justification
> > than "no need to enable".
> 
> That's why I asked to merge the comment from Dmitry:
> 
> "...moreover his makes PCIe EP fail on some of the platforms as powering on PHY
> requires refclk from the RC side, which is not enabled at the probe time."

The patch was posted and described basically as a cleanup of something
that was unnecessary but not harmful, which is not post-merge window
material.

If it is in fact a critical fix, that needs to be the central point of
the commit log, not something tacked on with "moreover" or
"additionally".  And we need something like a Fixes: tag so we know
when the problem was introduced and where to backport this.

Bjorn