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

From: Niklas Cassel <cassel@kernel.org>
To: Shawn Lin <shawn.lin@rock-chips.com>
Cc: "Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	linux-pci@vger.kernel.org, linux-rockchip@lists.infradead.org
Subject: Re: [PATCH] PCI: dw-rockchip: Remove PCIE_L0S_ENTRY check from rockchip_pcie_link_up()
Date: Wed, 9 Apr 2025 11:19:03 +0200	[thread overview]
Message-ID: <Z_Y7h1vzVCCEiXK6@ryzen> (raw)
In-Reply-To: <38e69551-cc40-11a9-191f-de9a193c5e51@rock-chips.com>

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).

Kind regards,
Niklas

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip