Re: [PATCH v7 1/2] PCI: designware-ep: Fix DBI access before core init

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

Hi Niklas,

On Tue, Nov 28, 2023 at 05:41:52PM +0000, Niklas Cassel wrote:
> On Mon, Nov 20, 2023 at 02:10:13PM +0530, Manivannan Sadhasivam wrote:
> > The drivers for platforms requiring reference clock from the PCIe host for
> > initializing their PCIe EP core, make use of the 'core_init_notifier'
> > feature exposed by the DWC common code. On these platforms, access to the
> > hw registers like DBI before completing the core initialization will result
> > in a whole system hang. But the current DWC EP driver tries to access DBI
> > registers during dw_pcie_ep_init() without waiting for core initialization
> > and it results in system hang on platforms making use of
> > 'core_init_notifier' such as Tegra194 and Qcom SM8450.
> > 
> > To workaround this issue, users of the above mentioned platforms have to
> > maintain the dependency with the PCIe host by booting the PCIe EP after
> > host boot. But this won't provide a good user experience, since PCIe EP is
> > _one_ of the features of those platforms and it doesn't make sense to
> > delay the whole platform booting due to the PCIe dependency.
> > 
> > So to fix this issue, let's move all the DBI access during
> > dw_pcie_ep_init() in the DWC EP driver to the dw_pcie_ep_init_complete()
> > API that gets called only after core initialization on these platforms.
> > This makes sure that the DBI register accesses are skipped during
> > dw_pcie_ep_init() and accessed later once the core initialization happens.
> > 
> > For the rest of the platforms, DBI access happens as usual.
> > 
> > Co-developed-by: Vidya Sagar <vidyas@nvidia.com>
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> 
> Hello Mani,
> 
> I tried this patch on top of a work in progress EP driver,
> which, similar to pcie-qcom-ep.c has a perst gpio as input,
> and a .core_init_notifier.
> 
> What I noticed is the following every time I reboot the RC, I get:
> 
> [  604.735115] debugfs: Directory 'a40000000.pcie_ep' with parent 'dmaengine' already present!
> [ 1000.713582] debugfs: Directory 'a40000000.pcie_ep' with parent 'dmaengine' already present!
> [ 1000.714355] debugfs: File 'mf' in directory '/' already present!
> [ 1000.714890] debugfs: File 'wr_ch_cnt' in directory '/' already present!
> [ 1000.715476] debugfs: File 'rd_ch_cnt' in directory '/' already present!
> [ 1000.716061] debugfs: Directory 'registers' with parent '/' already present!
> 
> 
> Also:
> 
> # ls -al /sys/class/dma/dma*/device | grep pcie
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma3chan0/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma3chan1/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma3chan2/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma3chan3/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma4chan0/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma4chan1/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma4chan2/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:14 /sys/class/dma/dma4chan3/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:16 /sys/class/dma/dma5chan0/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:16 /sys/class/dma/dma5chan1/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:16 /sys/class/dma/dma5chan2/device -> ../../../a40000000.pcie_ep
> lrwxrwxrwx    1 root     root             0 Jan  1 00:16 /sys/class/dma/dma5chan3/device -> ../../../a40000000.pcie_ep
> 
> Adds another dmaX entry for each reboot.
> 
> 
> I'm quite sure that you will see the same with pcie-qcom-ep.
> 

Yes, you are right. I'm aware of this issue but don't have a handy solution atm.

> I think that either the DWC drivers using core_init (only tegra and qcom)
> need to deinit the eDMA in their assert_perst() function, or this patch
> needs to deinit the eDMA before initializing it.
> 

The problem with first option is that it is too late. The moment EP gets perst
assert IRQ, refclk will be cut off by the host. So if EP tries to access any
hardware registers (edma_core_off) it will result in hard reboot.

The second option is not a complete solution, because the EPF drivers still need
to release the DMA channels and that can only happen if the EPF drivers know
that the host is going into power down state. Even if the DWC core makes an
assumption that EPF drivers are releasing the channel, in reality it is not
happening.

> 
> A problem with the current code, if you do NOT have this patch, which I assume
> is also problem on pcie-qcom-ep, is that since assert_perst() function performs
> a core reset, all the eDMA setting written in the dbi by the eDMA driver will be
> cleared, so a PERST assert + deassert by the RC will wipe the eDMA settings.
> Hopefully, this will no longer be a problem after this patch has been merged.
> 

Yes. Since all the DBI accesses were moved to the dw_pcie_ep_late_init()
function that get's called during perst deassert with the help of
dw_pcie_ep_init_complete(), all the settings will be reconfigured again.

- Mani

> 
> Kind regards,
> Niklas

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