Re: [PATCH] PCI: qcom-ep: Move controller cleanups to qcom_pcie_perst_deassert()

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Fri, Aug 16, 2024 at 02:12:22PM -0500, Bjorn Helgaas wrote:
> On Fri, Aug 16, 2024 at 10:30:29AM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Aug 15, 2024 at 05:47:17PM -0500, Bjorn Helgaas wrote:
> > > [+cc Vidya, Jon since tegra194 does similar things]
> > > 
> > > On Mon, Jul 29, 2024 at 05:52:45PM +0530, Manivannan Sadhasivam wrote:
> > > > Currently, the endpoint cleanup function dw_pcie_ep_cleanup() and EPF
> > > > deinit notify function pci_epc_deinit_notify() are called during the
> > > > execution of qcom_pcie_perst_assert() i.e., when the host has asserted
> > > > PERST#. But quickly after this step, refclk will also be disabled by the
> > > > host.
> > > > 
> > > > All of the Qcom endpoint SoCs supported as of now depend on the refclk from
> > > > the host for keeping the controller operational. Due to this limitation,
> > > > any access to the hardware registers in the absence of refclk will result
> > > > in a whole endpoint crash. Unfortunately, most of the controller cleanups
> > > > require accessing the hardware registers (like eDMA cleanup performed in
> > > > dw_pcie_ep_cleanup(), powering down MHI EPF etc...). So these cleanup
> > > > functions are currently causing the crash in the endpoint SoC once host
> > > > asserts PERST#.
> > > > 
> > > > One way to address this issue is by generating the refclk in the endpoint
> > > > itself and not depending on the host. But that is not always possible as
> > > > some of the endpoint designs do require the endpoint to consume refclk from
> > > > the host (as I was told by the Qcom engineers).
> > > > 
> > > > So let's fix this crash by moving the controller cleanups to the start of
> > > > the qcom_pcie_perst_deassert() function. qcom_pcie_perst_deassert() is
> > > > called whenever the host has deasserted PERST# and it is guaranteed that
> > > > the refclk would be active at this point. So at the start of this function,
> > > > the controller cleanup can be performed. Once finished, rest of the code
> > > > execution for PERST# deassert can continue as usual.
> > > 
> > > What makes this v6.11 material?  Does it fix a problem we added in
> > > v6.11-rc1?
> > 
> > No, this is not a 6.11 material, but the rest of the patches I
> > shared offline.
> 
> For reference, the patches you shared offline are:
> 
>   PCI: qcom: Use OPP only if the platform supports it
>   PCI: qcom-ep: Do not enable resources during probe()
>   PCI: qcom-ep: Disable MHI RAM data parity error interrupt for SA8775P SoC
>   PCI: qcom-ep: Move controller cleanups to qcom_pcie_perst_deassert()
> 

And then the note...

"last one is not strictly a 6.11 material, but rest are"

Sorry if that confused you. I shouldn't have mentioned this patch anyway.

> > > Is there a Fixes: commit?
> > 
> > Hmm, the controller addition commit could be the valid fixes tag.
> > 
> > > This patch essentially does this:
> > > 
> > >   qcom_pcie_perst_assert
> > > -   pci_epc_deinit_notify
> > > -   dw_pcie_ep_cleanup
> > >     qcom_pcie_disable_resources
> > > 
> > >   qcom_pcie_perst_deassert
> > > +   if (pcie_ep->cleanup_pending)
> > > +     pci_epc_deinit_notify(pci->ep.epc);
> > > +     dw_pcie_ep_cleanup(&pci->ep);
> > >     dw_pcie_ep_init_registers
> > >     pci_epc_init_notify
> > > 
> > > Maybe it makes sense to call both pci_epc_deinit_notify() and
> > > pci_epc_init_notify() from the PERST# deassert function, but it makes
> > > me question whether we really need both.
> > 
> > There is really no need to call pci_epc_deinit_notify() during the first
> > deassert (i.e., during the ep boot) because there are no cleanups to be done.
> > It is only needed during a successive PERST# assert + deassert.
> > 
> > > pcie-tegra194.c has a similar structure:
> > > 
> > >   pex_ep_event_pex_rst_assert
> > >     pci_epc_deinit_notify
> > >     dw_pcie_ep_cleanup
> > > 
> > >   pex_ep_event_pex_rst_deassert
> > >     dw_pcie_ep_init_registers
> > >     pci_epc_init_notify
> > > 
> > > Is there a reason to make them different, or could/should a similar
> > > change be made to tegra?
> > 
> > Design wise both drivers are similar, so it could apply. I didn't
> > spin a patch because if testing of tegra driver gets delayed (I've
> > seen this before), then I do not want to stall merging the whole
> > series. 
> 
> It can and should be separate patches, one per driver.  But I don't
> want to end up with the drivers being needlessly different.
> 

Ok. Let me spin a patch for that driver also.

> > For Qcom it is important to get this merged asap to avoid
> > the crash.
> 
> If this is not v6.11 material, there's time to work this out.
> 
> > > > +	if (pcie_ep->cleanup_pending) {
> > > 
> > > Do we really need this flag?  I assume the cleanup functions could
> > > tell whether any previous setup was done?
> > 
> > Not so. Some cleanup functions may trigger a warning if attempted to do it
> > before 'setup'. I think dw_edma_remove() that is part of dw_pcie_ep_cleanup()
> > does that IIRC.
> 
> It looks safe to me:
> 
>   dw_pcie_ep_cleanup
>     dw_pcie_edma_remove
>       dw_edma_remove(chip = &pci->edma)       # struct dw_pcie *pci
>         dev = chip->dev
>         dw = chip->dw
>         if (!dw)
>           return -ENODEV
> 
> but if not, it could probably be made safe by adding a NULL pointer
> check and/or a "chip->dw = NULL" at the right spot.
> 
> We hardly have any cleanup functions affected by "cleanup_pending", so
> I think we can decide that they should be safe before 'setup' and just
> make it so.
> 

I just tested by removing the cleanup flag and it doesn't seem to scream. Maybe
the issue I saw previously was unrelated.

- Mani

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