[PATCH] PCI: dw-rockchip: Remove PCIE_L0S_ENTRY check from rockchip_pcie_link_up()

[Manivannan Sadhasivam]

On Wed, Apr 09, 2025 at 11:19:03AM +0200, Niklas Cassel wrote:
> Hello Shawn,
> 
> On Wed, Apr 09, 2025 at 05:09:38PM +0800, Shawn Lin wrote:
> > 在 2025/04/09 星期三 16:30, Niklas Cassel 写道:
> > > On Wed, Apr 09, 2025 at 02:40:33PM +0800, Shawn Lin wrote:
> > > 
> > > Is there any advantage of using a rockchip specific way to read link up,
> > > rather than just reading link up via the DWC PCIE_PORT_DEBUG1 register?
> > 
> > This is a very good question which we tried but made real products
> > suffer from it for a long time, and finally we found the reason and
> > discarded it.
> > 
> > Quoted from DWC databook, section 8.2.3 AXI Bridge Initialization,
> > Clocking and Reset:
> > 
> > "In RC Mode, your AXI application must not generate any MEM or I/O
> > requests, until the host software has enabled the Memory Space Enable
> > (MSE), and IO Space Enable (ISE) bits respectively. Your RC application
> > should not generate CFG requests until it has confirmed that the link is
> > up by sampling the smlh_link_up and rdlh_link_up outputs."
> > 
> > Quoted from DWC databook, section 5.50 SII: Debug Signals
> > "[36]: smlh_link_up: LTSSM reports PHY link up or LTSSM is in
> > Loopback.Active for Loopback Master" which refers to
> > PCIE_PORT_DEBUG1_LINK_UP per code.
> > 
> > The timing in dwc core is drving smlh_link_up->L0->rdlh_link_up->FC
> > init(a fixed delay) from IC simulation when linking up.
> > 
> > The dw_pcie_link_up() wasn't reliably work as expected by massive test.
> > The problem is clear from our ASIC view, that cxpl_debug_info from DWC
> > core is missing rdlh_link_up. cxpl_debug_info[32:63] is indentical to
> > PCIE_PORT_DEBUG1, So reading PCIE_PORT_DEBUG1 and check
> > smlh_link_up isn't enough.
> > 
> > The problem was introduced by commit 1 and fixed by commit 2 but not to
> > the end. And finally commit 3 rename the register but not fix anything.
> > 
> > It was broken from the first time. Any dwc controllers should not be use
> > the buggy default method to check link up state from our view.
> > So this's the whole story for it, which may help you understand the
> > indeed problem and why we reinvent rockchip_pcie_link_up() here.
> > 
> > [1]. commit dac29e6c5460 ("PCI: designware: Add default link up check if
> >     sub-driver doesn't override")
> > 
> > [2]. commit 01c076732e82 ("PCI: designware: Check LTSSM training bit
> >     before deciding link is up")
> > 
> > [3]. commit 60ef4b072ba0 ("PCI: dwc: imx6: Share PHY debug register
> >     definitions")
> 
> Thank you for the detailed answer.
> 
> It seems like we should really add a warning and a comment in
> dw_pcie_link_up(), so that others don't get hit by this hard to debug issue!
> 

Right. But I'm also wondering if we should use the 'Data Link Layer Link Active'
bit in PCI Express Capability for checking link up. Qcom driver has been using
it from the start and there are no reported issues. We could add this as the
first fallback if the link_up callback is not provided.

- Mani

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