Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221107081327.336239-1-nathan@nathanrossi.com> <20221107084346.ksdhcxk7bppvpmqc@pali>
In-Reply-To: <20221107084346.ksdhcxk7bppvpmqc@pali>
From:   Nathan Rossi <nathan@nathanrossi.com>
Date:   Mon, 7 Nov 2022 19:10:02 +1000
Message-ID: <CA+aJhH0k1zaP0sVNk98qviTiyCEAmQKO4MOD8LSiHiwuzh6-aA@mail.gmail.com>
Subject: Re: [PATCH] PCI: mvebu: Set Target Link Speed for 2.5GT downstream devices
To:     =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Cc:     linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, Nathan Rossi <nathan.rossi@digi.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 7 Nov 2022 at 18:43, Pali Roh=C3=A1r <pali@kernel.org> wrote:
>
> On Monday 07 November 2022 08:13:27 Nathan Rossi wrote:
> > From: Nathan Rossi <nathan.rossi@digi.com>
> >
> > 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@digi.com>
> > ---
> >  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/controlle=
r/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 =3D mvebu_readl(port, PCIE_CTRL_OFF);
> > @@ -339,6 +339,22 @@ static void mvebu_pcie_setup_hw(struct mvebu_pcie_=
port *port)
> >       unmask |=3D 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 config=
ured
> > +      * in order to avoid issues with the controller when the upstream=
 port
> > +      * is configured to support 2.5GT and 5GT and the downstream devi=
ce is
> > +      * linked at 2.5GT, retraining the link in this case causes the l=
ink to
> > +      * drop taking significant time to retrain.
> > +      */
> > +     lnksta =3D mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL) >>=
 16;
> > +     if ((lnksta & PCI_EXP_LNKSTA_CLS) =3D=3D 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 =3D mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_L=
NKCTL2);
> > +             lnkctl2 &=3D ~PCI_EXP_LNKCTL2_TLS;
> > +             lnkctl2 |=3D PCI_EXP_LNKCTL2_TLS_2_5GT;
> > +             mvebu_writel(port, lnkctl2, PCIE_CAP_PCIEXP + PCI_EXP_LNK=
CTL2);
> > +     }
> >  }
> >
> >  static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie =
*pcie,
> > ---
> > 2.37.2