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

[manivannan.sadhasivam@xxxxxxxxxx]

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

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