Re: [PATCH] PCI: mvebu: Set Target Link Speed for 2.5GT downstream devices

[Nathan Rossi]

On Mon, 7 Nov 2022 at 18:43, Pali Rohár <pali@xxxxxxxxxx> wrote:
>
> On Monday 07 November 2022 08:13:27 Nathan Rossi wrote:
> > From: Nathan Rossi <nathan.rossi@xxxxxxxx>
> >
> > There is a known issue with the mvebu PCIe controller when triggering
> > retraining of the link (via Link Control) where the link is dropped
> > completely causing significant delay in the renegotiation of the link.
> > This occurs only when the downstream device is 2.5GT and the upstream
> > port is configured to support both 2.5GT and 5GT.
> >
> > It is possible to prevent this link dropping by setting the associated
> > link speed in Target Link Speed of the Link Control 2 register. This
> > only needs to be done when the downstream is specifically 2.5GT.
> >
> > This change applies the required Target Link Speed value during
> > mvebu_pcie_setup_hw conditionally depending on the current link speed
> > from the Link Status register, only applying the change when the link
> > is configured to 2.5GT already.
> >
> > Signed-off-by: Nathan Rossi <nathan.rossi@xxxxxxxx>
> > ---
> >  drivers/pci/controller/pci-mvebu.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> > index 1ced73726a..6a869a33ba 100644
> > --- a/drivers/pci/controller/pci-mvebu.c
> > +++ b/drivers/pci/controller/pci-mvebu.c
> > @@ -248,7 +248,7 @@ static void mvebu_pcie_setup_wins(struct mvebu_pcie_port *port)
> >
> >  static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
> >  {
> > -     u32 ctrl, lnkcap, cmd, dev_rev, unmask, sspl;
> > +     u32 ctrl, lnkcap, cmd, dev_rev, unmask, sspl, lnksta, lnkctl2;
> >
> >       /* Setup PCIe controller to Root Complex mode. */
> >       ctrl = mvebu_readl(port, PCIE_CTRL_OFF);
> > @@ -339,6 +339,22 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_port *port)
> >       unmask |= PCIE_INT_INTX(0) | PCIE_INT_INTX(1) |
> >                 PCIE_INT_INTX(2) | PCIE_INT_INTX(3);
> >       mvebu_writel(port, unmask, PCIE_INT_UNMASK_OFF);
> > +
> > +     /*
> > +      * Set Target Link Speed within the Link Control 2 register when the
> > +      * linked downstream device is connected at 2.5GT. This is configured
> > +      * in order to avoid issues with the controller when the upstream port
> > +      * is configured to support 2.5GT and 5GT and the downstream device is
> > +      * linked at 2.5GT, retraining the link in this case causes the link to
> > +      * drop taking significant time to retrain.
> > +      */
> > +     lnksta = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL) >> 16;
> > +     if ((lnksta & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB) {
>
> This code does not work because at this stage endpoint device does not
> have to be ready and therefore link is not established yet.
>
> Also this code is not running when kernel issue PCIe Hot Reset via
> PCI Secondary Bus Reset bit.
>
> And it does not handle possible hot-plug situation.
>
> That check that code below has to be done _after_ kernel enumerate
> device. PCI core code has already logic to handle delays for "slow"
> devices.
>
> And reverse operation (setting lnkctl2 target speed to original value)
> has to be called after unplugging device - when link goes down.
>
> If you want to work on this stuff, I can try to find my notes which I
> done during investigation of this issue... where is probably the best
> place in kernel pci core code for handling this issue.

Some notes/direction for implementation would be very appreciated. I
am not particularly familiar with the pci core code, so I don't have a
good idea on how to best implement this workaround.

Thanks,
Nathan

>
> > +             lnkctl2 = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL2);
> > +             lnkctl2 &= ~PCI_EXP_LNKCTL2_TLS;
> > +             lnkctl2 |= PCI_EXP_LNKCTL2_TLS_2_5GT;
> > +             mvebu_writel(port, lnkctl2, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL2);
> > +     }
> >  }
> >
> >  static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie,
> > ---
> > 2.37.2