Message-ID: <20240822064823.x26bjqev6ye32v5j@thinkpad>
Date: Thu, 22 Aug 2024 12:18:23 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Krzysztof Wilczyński <kw@...ux.com>,
	lpieralisi@...nel.org, robh@...nel.org, bhelgaas@...gle.com,
	linux-arm-msm@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] PCI: qcom-ep: Do not enable resources during probe()

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

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

Powered by blists - more mailing lists