lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 16 Dec 2020 11:01:51 -0600
From:   Rob Herring <robh@...nel.org>
To:     Kishon Vijay Abraham I <kishon@...com>
Cc:     Tom Joseph <tjoseph@...ence.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Nadeem Athani <nadeem@...ence.com>,
        linux-omap <linux-omap@...r.kernel.org>,
        PCI <linux-pci@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Milind Parab <mparab@...ence.com>,
        Swapnil Kashinath Jakhade <sjakhade@...ence.com>,
        Parshuram Raju Thombare <pthombar@...ence.com>
Subject: Re: [PATCH v5] PCI: cadence: Retrain Link to work around Gen2
 training defect.

On Wed, Dec 16, 2020 at 9:01 AM Kishon Vijay Abraham I <kishon@...com> wrote:
>
> Hi Rob,
>
> On 15/12/20 9:23 pm, Rob Herring wrote:
> > On Tue, Dec 15, 2020 at 1:00 AM Kishon Vijay Abraham I <kishon@...com> wrote:
> >>
> >> From: Nadeem Athani <nadeem@...ence.com>
> >>
> >> Cadence controller will not initiate autonomous speed change if strapped as
> >> Gen2. The Retrain Link bit is set as quirk to enable this speed change.
> >>
> >> Signed-off-by: Nadeem Athani <nadeem@...ence.com>
> >> [kishon@...com: Enable the workaround for TI's J721E SoC]
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@...com>
> >> ---
> >> Hi Lorenzo,
> >> The previous version of the patch can be found at [1].
> >> I slightly re-worked the patch from Nadeem
> >> *) Removed additional Link Up Check
> >> *) Removed quirk from pcie-cadence-plat.c
> >> *) Also removed additional compatible
> >>    "cdns,cdns-pcie-host-quirk-retrain" added in that series
> >> *) Enabled the quirk for J721E
> >> [1] -> http://lore.kernel.org/r/20201211144236.3825-1-nadeem@cadence.com
> >>
> >>  drivers/pci/controller/cadence/pci-j721e.c    |  3 +
> >>  .../controller/cadence/pcie-cadence-host.c    | 67 ++++++++++++++-----
> >>  drivers/pci/controller/cadence/pcie-cadence.h | 11 ++-
> >>  3 files changed, 62 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> >> index dac1ac8a7615..baf729850cb1 100644
> >> --- a/drivers/pci/controller/cadence/pci-j721e.c
> >> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> >> @@ -64,6 +64,7 @@ enum j721e_pcie_mode {
> >>
> >>  struct j721e_pcie_data {
> >>         enum j721e_pcie_mode    mode;
> >> +       bool                    quirk_retrain_flag;
> >>  };
> >>
> >>  static inline u32 j721e_pcie_user_readl(struct j721e_pcie *pcie, u32 offset)
> >> @@ -280,6 +281,7 @@ static struct pci_ops cdns_ti_pcie_host_ops = {
> >>
> >>  static const struct j721e_pcie_data j721e_pcie_rc_data = {
> >>         .mode = PCI_MODE_RC,
> >> +       .quirk_retrain_flag = true,
> >>  };
> >>
> >>  static const struct j721e_pcie_data j721e_pcie_ep_data = {
> >> @@ -388,6 +390,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >>
> >>                 bridge->ops = &cdns_ti_pcie_host_ops;
> >>                 rc = pci_host_bridge_priv(bridge);
> >> +               rc->quirk_retrain_flag = data->quirk_retrain_flag;
> >>
> >>                 cdns_pcie = &rc->pcie;
> >>                 cdns_pcie->dev = dev;
> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> index 811c1cb2e8de..773c0d1137ed 100644
> >> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> @@ -77,6 +77,50 @@ static struct pci_ops cdns_pcie_host_ops = {
> >>         .write          = pci_generic_config_write,
> >>  };
> >>
> >> +static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie)
> >> +{
> >> +       struct device *dev = pcie->dev;
> >> +       int retries;
> >> +
> >> +       /* Check if the link is up or not */
> >> +       for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> >> +               if (cdns_pcie_link_up(pcie)) {
> >> +                       dev_info(dev, "Link up\n");
> >> +                       return 0;
> >> +               }
> >> +               usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> >> +       }
> >> +
> >> +       return -ETIMEDOUT;
> >> +}
> >> +
> >> +static void cdns_pcie_retrain(struct cdns_pcie *pcie)
> >> +{
> >> +       u32 lnk_cap_sls, pcie_cap_off = CDNS_PCIE_RP_CAP_OFFSET;
> >> +       u16 lnk_stat, lnk_ctl;
> >> +
> >> +       /*
> >> +        * Set retrain bit if current speed is 2.5 GB/s,
> >> +        * but the PCIe root port support is > 2.5 GB/s.
> >
> > If you don't have the retrain quirk, wouldn't this condition never
> > happen and then the function is just a nop? So this could just be
> > called unconditionally.
>
> Yeah, but only for the quirk we have to retrain to go to GEN2 speed
> mode. Else the HW will automatically retrain and go to GEN2.

Again, so you don't need a flag for this. Comparing the speed is
enough. IOW, all you need is:

if (current speed < advertised speed)
  do retrain

The question is the condition ever true and you don't want to do a
retrain? I could see higher speeds being unstable or something, but
then 'advertised speed' would be lowered in that case (to prevent auto
retraining, right?) and the condition would be false.

Rob

Powered by blists - more mailing lists