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