[PATCH 1/1] PCI: armada8k: Disable LTSSM on link down interrupts

[PATCH 1/1] PCI: armada8k: Disable LTSSM on link down interrupts

[Jenishkumar Maheshbhai Patel]

When a PCI link down condition is detected, the link training state
machine must be disabled immediately.

Signed-off-by: Jenishkumar Maheshbhai Patel <jpatel2@marvell.com>
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index b5c599ccaacf..07775539b321 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -53,6 +53,10 @@ struct armada8k_pcie {
 #define PCIE_INT_C_ASSERT_MASK		BIT(11)
 #define PCIE_INT_D_ASSERT_MASK		BIT(12)
 
+#define PCIE_GLOBAL_INT_CAUSE2_REG	(PCIE_VENDOR_REGS_OFFSET + 0x24)
+#define PCIE_GLOBAL_INT_MASK2_REG	(PCIE_VENDOR_REGS_OFFSET + 0x28)
+#define PCIE_INT2_PHY_RST_LINK_DOWN	BIT(1)
+
 #define PCIE_ARCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET + 0x50)
 #define PCIE_AWCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET + 0x54)
 #define PCIE_ARUSER_REG			(PCIE_VENDOR_REGS_OFFSET + 0x5C)
@@ -204,6 +208,11 @@ static int armada8k_pcie_host_init(struct dw_pcie_rp *pp)
 	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
 	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
 
+	/* Also enable link down interrupts */
+	reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
+	reg |= PCIE_INT2_PHY_RST_LINK_DOWN;
+	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
+
 	return 0;
 }
 
@@ -221,6 +230,35 @@ static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg)
 	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
 	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
 
+	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG);
+
+	if (PCIE_INT2_PHY_RST_LINK_DOWN & val) {
+		u32 ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);
+		/*
+		 * The link went down. Disable LTSSM immediately. This
+		 * unlocks the root complex config registers. Downstream
+		 * device accesses will return all-Fs
+		 */
+		ctrl_reg &= ~(PCIE_APP_LTSSM_EN);
+		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, ctrl_reg);
+		/*
+		 * Mask link down interrupts. They can be re-enabled once
+		 * the link is retrained.
+		 */
+		ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
+		ctrl_reg &= ~PCIE_INT2_PHY_RST_LINK_DOWN;
+		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, ctrl_reg);
+		/*
+		 * At this point a worker thread can be triggered to
+		 * initiate a link retrain. If link retrains were
+		 * possible, that is.
+		 */
+		dev_dbg(pci->dev, "%s: link went down\n", __func__);
+	}
+
+	/* Now clear the second interrupt cause. */
+	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG, val);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.25.1

Re: [PATCH 1/1] PCI: armada8k: Disable LTSSM on link down interrupts

[Bjorn Helgaas]

On Mon, Nov 11, 2024 at 10:42:41PM -0800, Jenishkumar Maheshbhai Patel wrote:
> When a PCI link down condition is detected, the link training state
> machine must be disabled immediately.

Why?

"Immediately" has no meaning here.  Arbitrary delays are possible and
must not break anything.

> Signed-off-by: Jenishkumar Maheshbhai Patel <jpatel2@marvell.com>
> ---
>  drivers/pci/controller/dwc/pcie-armada8k.c | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index b5c599ccaacf..07775539b321 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -53,6 +53,10 @@ struct armada8k_pcie {
>  #define PCIE_INT_C_ASSERT_MASK		BIT(11)
>  #define PCIE_INT_D_ASSERT_MASK		BIT(12)
>  
> +#define PCIE_GLOBAL_INT_CAUSE2_REG	(PCIE_VENDOR_REGS_OFFSET + 0x24)
> +#define PCIE_GLOBAL_INT_MASK2_REG	(PCIE_VENDOR_REGS_OFFSET + 0x28)
> +#define PCIE_INT2_PHY_RST_LINK_DOWN	BIT(1)
> +
>  #define PCIE_ARCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET + 0x50)
>  #define PCIE_AWCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET + 0x54)
>  #define PCIE_ARUSER_REG			(PCIE_VENDOR_REGS_OFFSET + 0x5C)
> @@ -204,6 +208,11 @@ static int armada8k_pcie_host_init(struct dw_pcie_rp *pp)
>  	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
>  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
>  
> +	/* Also enable link down interrupts */
> +	reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> +	reg |= PCIE_INT2_PHY_RST_LINK_DOWN;
> +	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
> +
>  	return 0;
>  }
>  
> @@ -221,6 +230,35 @@ static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg)
>  	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
>  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
>  
> +	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG);
> +
> +	if (PCIE_INT2_PHY_RST_LINK_DOWN & val) {
> +		u32 ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);

Add blank line.

> +		/*
> +		 * The link went down. Disable LTSSM immediately. This
> +		 * unlocks the root complex config registers. Downstream
> +		 * device accesses will return all-Fs
> +		 */
> +		ctrl_reg &= ~(PCIE_APP_LTSSM_EN);
> +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, ctrl_reg);

And here.

> +		/*
> +		 * Mask link down interrupts. They can be re-enabled once
> +		 * the link is retrained.
> +		 */
> +		ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> +		ctrl_reg &= ~PCIE_INT2_PHY_RST_LINK_DOWN;
> +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, ctrl_reg);

And here.  Follow existing coding style in this file.

> +		/*
> +		 * At this point a worker thread can be triggered to
> +		 * initiate a link retrain. If link retrains were
> +		 * possible, that is.
> +		 */
> +		dev_dbg(pci->dev, "%s: link went down\n", __func__);
> +	}
> +
> +	/* Now clear the second interrupt cause. */
> +	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG, val);
> +
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 2.25.1
> 

RE: [PATCH 1/1] PCI: armada8k: Disable LTSSM on link down interrupts

[Wilson Ding]

I am writing to follow up on this serious of PCIe patches submitted by Jenish.
Unfortunately, he has since left the company, and some comments on these
 patches were not addressed in a timely manner. I apologize for the delay
and any inconvenience this may have caused. I have reviewed the feedback
provided and am now taking over this work. I appreciate the time and effort
you have put into reviewing the patch and providing valuable comments.
I will ensure that the necessary changes are made and resubmit the patch
in the proper manner, as it was not initially submitted as a series.

> > When a PCI link down condition is detected, the link training state
> > machine must be disabled immediately.
> 
> Why?
> 
> "Immediately" has no meaning here.  Arbitrary delays are possible and must
> not break anything.

Yes, I agree. A delay cannot be avoided. The issue we encountered is that
the RC may not be aware of the link down when it happens. In this case,
any access to the PCI config space may hang up PCI bus. The only thing we
can do is to rely on this link down interrupt. By disabling APP_LTSSM_EN,
RC will bypass all device accesses with returning all ones (for read).

> > Signed-off-by: Jenishkumar Maheshbhai Patel
> > <mailto:jpatel2@marvell.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-armada8k.c | 38
> > ++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c
> > b/drivers/pci/controller/dwc/pcie-armada8k.c
> > index b5c599ccaacf..07775539b321 100644
> > --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> > +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> > @@ -53,6 +53,10 @@ struct armada8k_pcie {
> >  #define PCIE_INT_C_ASSERT_MASK		BIT(11)
> >  #define PCIE_INT_D_ASSERT_MASK		BIT(12)
> >
> > +#define PCIE_GLOBAL_INT_CAUSE2_REG	(PCIE_VENDOR_REGS_OFFSET
> + 0x24)
> > +#define PCIE_GLOBAL_INT_MASK2_REG	(PCIE_VENDOR_REGS_OFFSET
> + 0x28)
> > +#define PCIE_INT2_PHY_RST_LINK_DOWN	BIT(1)
> > +
> >  #define PCIE_ARCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET
> + 0x50)
> >  #define PCIE_AWCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET
> + 0x54)
> >  #define PCIE_ARUSER_REG			(PCIE_VENDOR_REGS_OFFSET
> + 0x5C)
> > @@ -204,6 +208,11 @@ static int armada8k_pcie_host_init(struct
> dw_pcie_rp *pp)
> >  	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
> >  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
> >
> > +	/* Also enable link down interrupts */
> > +	reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> > +	reg |= PCIE_INT2_PHY_RST_LINK_DOWN;
> > +	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
> > +
> >  	return 0;
> >  }
> >
> > @@ -221,6 +230,35 @@ static irqreturn_t armada8k_pcie_irq_handler(int
> irq, void *arg)
> >  	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
> >  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
> >
> > +	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG);
> > +
> > +	if (PCIE_INT2_PHY_RST_LINK_DOWN & val) {
> > +		u32 ctrl_reg = dw_pcie_readl_dbi(pci,
> PCIE_GLOBAL_CONTROL_REG);
> 
> Add blank line.
> 
> > +		/*
> > +		 * The link went down. Disable LTSSM immediately. This
> > +		 * unlocks the root complex config registers. Downstream
> > +		 * device accesses will return all-Fs
> > +		 */
> > +		ctrl_reg &= ~(PCIE_APP_LTSSM_EN);
> > +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG,
> ctrl_reg);
> 
> And here.
> 
> > +		/*
> > +		 * Mask link down interrupts. They can be re-enabled once
> > +		 * the link is retrained.
> > +		 */
> > +		ctrl_reg = dw_pcie_readl_dbi(pci,
> PCIE_GLOBAL_INT_MASK2_REG);
> > +		ctrl_reg &= ~PCIE_INT2_PHY_RST_LINK_DOWN;
> > +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG,
> ctrl_reg);
> 
> And here.  Follow existing coding style in this file.
> 
> > +		/*
> > +		 * At this point a worker thread can be triggered to
> > +		 * initiate a link retrain. If link retrains were
> > +		 * possible, that is.
> > +		 */
> > +		dev_dbg(pci->dev, "%s: link went down\n", __func__);
> > +	}
> > +
> > +	/* Now clear the second interrupt cause. */
> > +	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG, val);
> > +
> >  	return IRQ_HANDLED;
> >  }
> >
> > --
> > 2.25.1
> >

Re: [PATCH 1/1] PCI: armada8k: Disable LTSSM on link down interrupts

[manivannan.sadhasivam]

On Sat, Feb 01, 2025 at 12:57:56AM +0000, Wilson Ding wrote:
> I am writing to follow up on this serious of PCIe patches submitted by Jenish.
> Unfortunately, he has since left the company, and some comments on these
>  patches were not addressed in a timely manner. I apologize for the delay
> and any inconvenience this may have caused. I have reviewed the feedback
> provided and am now taking over this work. I appreciate the time and effort
> you have put into reviewing the patch and providing valuable comments.
> I will ensure that the necessary changes are made and resubmit the patch
> in the proper manner, as it was not initially submitted as a series.
> 
> > > When a PCI link down condition is detected, the link training state
> > > machine must be disabled immediately.
> > 
> > Why?
> > 
> > "Immediately" has no meaning here.  Arbitrary delays are possible and must
> > not break anything.
> 
> Yes, I agree. A delay cannot be avoided. The issue we encountered is that
> the RC may not be aware of the link down when it happens. In this case,
> any access to the PCI config space may hang up PCI bus. The only thing we
> can do is to rely on this link down interrupt. By disabling APP_LTSSM_EN,
> RC will bypass all device accesses with returning all ones (for read).
> 

Ok. One comment below.

> > > Signed-off-by: Jenishkumar Maheshbhai Patel
> > > <mailto:jpatel2@marvell.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-armada8k.c | 38
> > > ++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c
> > > b/drivers/pci/controller/dwc/pcie-armada8k.c
> > > index b5c599ccaacf..07775539b321 100644
> > > --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> > > +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> > > @@ -53,6 +53,10 @@ struct armada8k_pcie {
> > >  #define PCIE_INT_C_ASSERT_MASK		BIT(11)
> > >  #define PCIE_INT_D_ASSERT_MASK		BIT(12)
> > >
> > > +#define PCIE_GLOBAL_INT_CAUSE2_REG	(PCIE_VENDOR_REGS_OFFSET
> > + 0x24)
> > > +#define PCIE_GLOBAL_INT_MASK2_REG	(PCIE_VENDOR_REGS_OFFSET
> > + 0x28)
> > > +#define PCIE_INT2_PHY_RST_LINK_DOWN	BIT(1)
> > > +
> > >  #define PCIE_ARCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET
> > + 0x50)
> > >  #define PCIE_AWCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET
> > + 0x54)
> > >  #define PCIE_ARUSER_REG			(PCIE_VENDOR_REGS_OFFSET
> > + 0x5C)
> > > @@ -204,6 +208,11 @@ static int armada8k_pcie_host_init(struct
> > dw_pcie_rp *pp)
> > >  	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
> > >  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
> > >
> > > +	/* Also enable link down interrupts */
> > > +	reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> > > +	reg |= PCIE_INT2_PHY_RST_LINK_DOWN;
> > > +	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -221,6 +230,35 @@ static irqreturn_t armada8k_pcie_irq_handler(int
> > irq, void *arg)
> > >  	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
> > >  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
> > >
> > > +	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG);
> > > +
> > > +	if (PCIE_INT2_PHY_RST_LINK_DOWN & val) {
> > > +		u32 ctrl_reg = dw_pcie_readl_dbi(pci,
> > PCIE_GLOBAL_CONTROL_REG);
> > 
> > Add blank line.
> > 
> > > +		/*
> > > +		 * The link went down. Disable LTSSM immediately. This
> > > +		 * unlocks the root complex config registers. Downstream
> > > +		 * device accesses will return all-Fs
> > > +		 */
> > > +		ctrl_reg &= ~(PCIE_APP_LTSSM_EN);
> > > +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG,
> > ctrl_reg);
> > 
> > And here.
> > 
> > > +		/*
> > > +		 * Mask link down interrupts. They can be re-enabled once
> > > +		 * the link is retrained.
> > > +		 */
> > > +		ctrl_reg = dw_pcie_readl_dbi(pci,
> > PCIE_GLOBAL_INT_MASK2_REG);
> > > +		ctrl_reg &= ~PCIE_INT2_PHY_RST_LINK_DOWN;
> > > +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG,
> > ctrl_reg);
> > 
> > And here.  Follow existing coding style in this file.
> > 
> > > +		/*
> > > +		 * At this point a worker thread can be triggered to
> > > +		 * initiate a link retrain. If link retrains were
> > > +		 * possible, that is.
> > > +		 */

Who is supposed to retrain the link? Where is the worker thread?

- Mani

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

RE: [EXTERNAL] Re: [PATCH 1/1] PCI: armada8k: Disable LTSSM on link down interrupts

[Wilson Ding]

> -----Original Message-----
> From: manivannan.sadhasivam@linaro.org
> <manivannan.sadhasivam@linaro.org>
> Sent: Friday, February 7, 2025 9:34 AM
> To: Wilson Ding <dingwei@marvell.com>
> Cc: Bjorn Helgaas <helgaas@kernel.org>; lpieralisi@kernel.org;
> thomas.petazzoni@bootlin.com; kw@linux.com; robh@kernel.org;
> bhelgaas@google.com; linux-pci@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sanghoon Lee
> <salee@marvell.com>
> Subject: [EXTERNAL] Re: [PATCH 1/1] PCI: armada8k: Disable LTSSM on link
> down interrupts
> 
> On Sat, Feb 01, 2025 at 12: 57: 56AM +0000, Wilson Ding wrote: > I am
> writing to follow up on this serious of PCIe patches submitted by Jenish. >
> Unfortunately, he has since left the company, and some comments on these >
> patches were 
> On Sat, Feb 01, 2025 at 12:57:56AM +0000, Wilson Ding wrote:
> > I am writing to follow up on this serious of PCIe patches submitted by Jenish.
> > Unfortunately, he has since left the company, and some comments on
> > these  patches were not addressed in a timely manner. I apologize for
> > the delay and any inconvenience this may have caused. I have reviewed
> > the feedback provided and am now taking over this work. I appreciate
> > the time and effort you have put into reviewing the patch and providing
> valuable comments.
> > I will ensure that the necessary changes are made and resubmit the
> > patch in the proper manner, as it was not initially submitted as a series.
> >
> > > > When a PCI link down condition is detected, the link training
> > > > state machine must be disabled immediately.
> > >
> > > Why?
> > >
> > > "Immediately" has no meaning here.  Arbitrary delays are possible
> > > and must not break anything.
> >
> > Yes, I agree. A delay cannot be avoided. The issue we encountered is
> > that the RC may not be aware of the link down when it happens. In this
> > case, any access to the PCI config space may hang up PCI bus. The only
> > thing we can do is to rely on this link down interrupt. By disabling
> > APP_LTSSM_EN, RC will bypass all device accesses with returning all ones (for
> read).
> >
> 
> Ok. One comment below.
> 
> > > > Signed-off-by: Jenishkumar Maheshbhai Patel
> > > > <mailto:jpatel2@marvell.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-armada8k.c | 38
> > > > ++++++++++++++++++++++
> > > >  1 file changed, 38 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c
> > > > b/drivers/pci/controller/dwc/pcie-armada8k.c
> > > > index b5c599ccaacf..07775539b321 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> > > > @@ -53,6 +53,10 @@ struct armada8k_pcie {
> > > >  #define PCIE_INT_C_ASSERT_MASK		BIT(11)
> > > >  #define PCIE_INT_D_ASSERT_MASK		BIT(12)
> > > >
> > > > +#define PCIE_GLOBAL_INT_CAUSE2_REG	(PCIE_VENDOR_REGS_OFFSET
> > > + 0x24)
> > > > +#define PCIE_GLOBAL_INT_MASK2_REG	(PCIE_VENDOR_REGS_OFFSET
> > > + 0x28)
> > > > +#define PCIE_INT2_PHY_RST_LINK_DOWN	BIT(1)
> > > > +
> > > >  #define PCIE_ARCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET
> > > + 0x50)
> > > >  #define PCIE_AWCACHE_TRC_REG		(PCIE_VENDOR_REGS_OFFSET
> > > + 0x54)
> > > >  #define PCIE_ARUSER_REG
> 	(PCIE_VENDOR_REGS_OFFSET
> > > + 0x5C)
> > > > @@ -204,6 +208,11 @@ static int armada8k_pcie_host_init(struct
> > > dw_pcie_rp *pp)
> > > >  	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
> > > >  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
> > > >
> > > > +	/* Also enable link down interrupts */
> > > > +	reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> > > > +	reg |= PCIE_INT2_PHY_RST_LINK_DOWN;
> > > > +	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >
> > > > @@ -221,6 +230,35 @@ static irqreturn_t
> > > > armada8k_pcie_irq_handler(int
> > > irq, void *arg)
> > > >  	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
> > > >  	dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
> > > >
> > > > +	val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG);
> > > > +
> > > > +	if (PCIE_INT2_PHY_RST_LINK_DOWN & val) {
> > > > +		u32 ctrl_reg = dw_pcie_readl_dbi(pci,
> > > PCIE_GLOBAL_CONTROL_REG);
> > >
> > > Add blank line.
> > >
> > > > +		/*
> > > > +		 * The link went down. Disable LTSSM immediately. This
> > > > +		 * unlocks the root complex config registers. Downstream
> > > > +		 * device accesses will return all-Fs
> > > > +		 */
> > > > +		ctrl_reg &= ~(PCIE_APP_LTSSM_EN);
> > > > +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG,
> > > ctrl_reg);
> > >
> > > And here.
> > >
> > > > +		/*
> > > > +		 * Mask link down interrupts. They can be re-enabled once
> > > > +		 * the link is retrained.
> > > > +		 */
> > > > +		ctrl_reg = dw_pcie_readl_dbi(pci,
> > > PCIE_GLOBAL_INT_MASK2_REG);
> > > > +		ctrl_reg &= ~PCIE_INT2_PHY_RST_LINK_DOWN;
> > > > +		dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG,
> > > ctrl_reg);
> > >
> > > And here.  Follow existing coding style in this file.
> > >
> > > > +		/*
> > > > +		 * At this point a worker thread can be triggered to
> > > > +		 * initiate a link retrain. If link retrains were
> > > > +		 * possible, that is.
> > > > +		 */
> 
> Who is supposed to retrain the link? Where is the worker thread?
> 
> - Mani

The worker thread is added by another patch posted separately. And you
have some comments on that thread ([PATCH 1/1] PCI: armada8k: Add link-
down handle) regarding removing and rescanning the root port. I am sorry
for the mess and confusions. I am fixing the comments received previously
and will push a new version of patches as a series next week.

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