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

[Wilson Ding]

> -----Original Message-----
> From: manivannan.sadhasivam@xxxxxxxxxx
> <manivannan.sadhasivam@xxxxxxxxxx>
> Sent: Friday, February 7, 2025 9:34 AM
> To: Wilson Ding <dingwei@xxxxxxxxxxx>
> Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx>; lpieralisi@xxxxxxxxxx;
> thomas.petazzoni@xxxxxxxxxxx; kw@xxxxxxxxx; robh@xxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sanghoon Lee
> <salee@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> > > > ---
> > > >  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.

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