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 -- மணிவண்ணன் சதாசிவம்
