[<prev] [next>] [day] [month] [year] [list]
Message-ID:
<BY3PR18MB467343D941D2F7706FD88FCBA7EB2@BY3PR18MB4673.namprd18.prod.outlook.com>
Date: Sat, 1 Feb 2025 00:57:56 +0000
From: Wilson Ding <dingwei@...vell.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "lpieralisi@...nel.org" <lpieralisi@...nel.org>,
"thomas.petazzoni@...tlin.com" <thomas.petazzoni@...tlin.com>,
"kw@...ux.com"
<kw@...ux.com>,
"manivannan.sadhasivam@...aro.org"
<manivannan.sadhasivam@...aro.org>,
"robh@...nel.org" <robh@...nel.org>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"linux-pci@...r.kernel.org"
<linux-pci@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
Sanghoon Lee <salee@...vell.com>
Subject: RE: [PATCH 1/1] PCI: armada8k: Disable LTSSM on link down interrupts
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@...vell.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
> >
Powered by blists - more mailing lists