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

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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.

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.


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.


Kind regards,
Niklas