Re: [PATCH v2] PCI: dw-rockchip: Delay link training after hot reset in EP mode

From: Niklas Cassel <cassel@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kwilczynski@kernel.org>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Wilfred Mallawa" <wilfred.mallawa@wdc.com>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH v2] PCI: dw-rockchip: Delay link training after hot reset in EP mode
Date: Wed, 18 Jun 2025 16:40:11 +0200	[thread overview]
Message-ID: <aFLPyxZD5lkAtw6b@ryzen> (raw)
In-Reply-To: <aFLL0vPFzxdVOR9a@ryzen>

On Wed, Jun 18, 2025 at 04:23:19PM +0200, Niklas Cassel wrote:
> On Tue, Jun 17, 2025 at 05:05:23PM -0500, Bjorn Helgaas wrote:
> > On Tue, Jun 17, 2025 at 05:01:16PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Jun 13, 2025 at 12:19:09PM +0200, Niklas Cassel wrote:
> > 
> > Oh, and this sets PCIE_LTSSM_ENABLE_ENHANCE | PCIE_LTSSM_APP_DLY2_EN
> > once at probe-time, but what about after a link-down/link-up cycle?
> > 
> > Don't we need to set PCIE_LTSSM_ENABLE_ENHANCE |
> > PCIE_LTSSM_APP_DLY2_EN again when the link comes up so we don't have
> > the same race when the link goes down again?
> 
> Nope, we don't.
> 
> To verify I used this patch:
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index be239254aacd..e79add5412b8 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -506,6 +506,8 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
>         if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
>                 dev_dbg(dev, "hot reset or link-down reset\n");
>                 dw_pcie_ep_linkdown(&pci->ep);
> +               pr_info("PCIE_CLIENT_HOT_RESET_CTRL after reset: %#x\n",
> +                       rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL));
>                 /* Stop delaying link training. */
>                 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_APP_DLY2_DONE);
>                 rockchip_pcie_writel_apb(rockchip, val,
> 
> 
> 
> 
> [   85.979567] rockchip-dw-pcie a40000000.pcie-ep: hot reset or link-down reset
> [   85.980210] PCIE_CLIENT_HOT_RESET_CTRL after reset: 0x12
> [   93.720413] rockchip-dw-pcie a40000000.pcie-ep: hot reset or link-down reset
> [   93.721074] PCIE_CLIENT_HOT_RESET_CTRL after reset: 0x12
> 
> 0x12 == bit 1 and bit 4 are set.
> 
> bit 1: app_dly2_en
> bit 4: app_ltssm_enable_enhance

Oh, and just to verify that the hardware does not clear the app_dly2_en bit
when we write the app_dly2_done bit, I ran the same test, but with the
prints in rockchip_pcie_ep_sys_irq_thread(), just after calling
dw_pcie_ep_linkup(&pci->ep); and got the same result:

[   57.176862] rockchip-dw-pcie a40000000.pcie-ep: link up
[   57.177338] PCIE_CLIENT_HOT_RESET_CTRL after linkup: 0x12
[   72.448052] rockchip-dw-pcie a40000000.pcie-ep: link up
[   72.448527] PCIE_CLIENT_HOT_RESET_CTRL after linkup: 0x12

So I think this patch is good as is.
(Except for the citation style in the commit message which you didn't like :))

Kind regards,
Niklas