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

[Shawn Lin]

在 2025/04/09 星期三 17:19, Niklas Cassel 写道:
> 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!
> 
> (Especially since dw_pcie_link_up() was added by someone with a @synopsys.com
> email).
> 

Yep, will do it with another patch.

> 
> Kind regards,
> Niklas
>