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
--
மணிவண்ணன் சதாசிவம்
