Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Athani Nadeem Ladkhan <nadeem@cadence.com>
To:     Bjorn Helgaas <helgaas@kernel.org>,
        Kishon Vijay Abraham I <kishon@ti.com>
CC:     Tom Joseph <tjoseph@cadence.com>, Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Milind Parab <mparab@cadence.com>,
        Swapnil Kashinath Jakhade <sjakhade@cadence.com>,
        Parshuram Raju Thombare <pthombar@cadence.com>
Subject: RE: [PATCH v5] PCI: cadence: Retrain Link to work around Gen2
 training defect.
Thread-Topic: [PATCH v5] PCI: cadence: Retrain Link to work around Gen2
 training defect.
Thread-Index: AQHW0q/zrZ1Sf6FZkk+47dkzT3Kdt6n4zyoAgAiIBeA=
Date:   Mon, 21 Dec 2020 09:55:56 +0000
Message-ID: <SN2PR07MB2557691709B30C5290141F99D8C00@SN2PR07MB2557.namprd07.prod.outlook.com>
References: <20201215070009.27937-1-kishon@ti.com>
 <20201215233028.GA336802@bjorn-Precision-5520>
In-Reply-To: <20201215233028.GA336802@bjorn-Precision-5520>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: eb790e42-d63f-4443-58f6-08d8a5969d0d
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Dec 2020 09:55:56.9950
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: elbkEgXdeCyOjKh2hIZuMEduJ6SvpZ0zB9TnJvAYdAMAJoi5OOjNhXJvcZNBf2/IE8wpqeqZVem4PPJeXZVfOIdgyk3RXCbn/V8TJR4w3+Y=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR07MB4768
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.343,18.0.737
 definitions=2020-12-21_02:2020-12-21,2020-12-21 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_check_notspam policy=outbound_check score=0 malwarescore=0
 suspectscore=0 adultscore=0 spamscore=0 mlxscore=0 mlxlogscore=999
 bulkscore=0 lowpriorityscore=0 phishscore=0 impostorscore=0
 priorityscore=1501 clxscore=1011 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2009150000 definitions=main-2012210072
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, December 16, 2020 5:00 AM
> To: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Tom Joseph <tjoseph@cadence.com>; Rob Herring <robh@kernel.org>;
> Bjorn Helgaas <bhelgaas@google.com>; Athani Nadeem Ladkhan
> <nadeem@cadence.com>; linux-omap@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Milind Parab <mparab@cadence.com>; Swapnil
> Kashinath Jakhade <sjakhade@cadence.com>; Parshuram Raju Thombare
> <pthombar@cadence.com>
> Subject: Re: [PATCH v5] PCI: cadence: Retrain Link to work around Gen2
> training defect.
>=20
> EXTERNAL MAIL
>=20
>=20
> On Tue, Dec 15, 2020 at 12:30:09PM +0530, Kishon Vijay Abraham I wrote:
> > From: Nadeem Athani <nadeem@cadence.com>
> >
> > Cadence controller will not initiate autonomous speed change if
> > strapped as Gen2. The Retrain Link bit is set as quirk to enable this s=
peed
> change.
> >
> > Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> > [kishon@ti.com: Enable the workaround for TI's J721E SoC]
> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.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] ->
> > https://urldefense.com/v3/__http://lore.kernel.org/r/20201211144236.38
> > 25-1-
> nadeem@cadence.com__;!!EHscmS1ygiU1lA!SAT97OdWrbRceVlkVfH048koT
> X-
> > ZjfJL85CdVQ1P6TCEOJ2TnJk06mGnd8fz1g$
> >
> >  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 =3D {
> >
> >  static const struct j721e_pcie_data j721e_pcie_rc_data =3D {
> >  	.mode =3D PCI_MODE_RC,
> > +	.quirk_retrain_flag =3D true,
> >  };
> >
> >  static const struct j721e_pcie_data j721e_pcie_ep_data =3D { @@ -388,6
> > +390,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
> >
> >  		bridge->ops =3D &cdns_ti_pcie_host_ops;
> >  		rc =3D pci_host_bridge_priv(bridge);
> > +		rc->quirk_retrain_flag =3D data->quirk_retrain_flag;
> >
> >  		cdns_pcie =3D &rc->pcie;
> >  		cdns_pcie->dev =3D 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 =3D {
> >  	.write		=3D pci_generic_config_write,
> >  };
> >
> > +static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie) {
> > +	struct device *dev =3D pcie->dev;
> > +	int retries;
> > +
> > +	/* Check if the link is up or not */
> > +	for (retries =3D 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> > +		if (cdns_pcie_link_up(pcie)) {
>=20
> - cdns_pcie_link_up() always returns "true" except for j721e_pcie_ops.
>   Is it really true that we can assume the link is up otherwise?
In cdns_pcie_link_up, the soc specific code should return the link status.
In this case, only TI j721e soc uses the driver.

>=20
> - Where did the LINK_WAIT_* values come from?  Are they derived from
>   something in the PCIe spec?
No, not derived from PCIe spec, but based on experimentation, working value=
.
This value is worst case timeout. Almost all the host controllers uses this=
 value.

>=20
> - Is the LINK_WAIT timeout related to LINK_RETRAIN_TIMEOUT?
Yes

>=20
> - If the PCI core does a link retrain, e.g., in pcie_retrain_link(),
>   does that work correctly on this device?
Yes
>=20
> [I see now that this patch only *moves* this code without changing it.
> But I'm still curious.]
>=20
> > +			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 =3D 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.
> > +	 */
> > +
> > +	lnk_cap_sls =3D cdns_pcie_readl(pcie, (CDNS_PCIE_RP_BASE +
> pcie_cap_off +
> > +					     PCI_EXP_LNKCAP));
> > +	if ((lnk_cap_sls & PCI_EXP_LNKCAP_SLS) <=3D
> PCI_EXP_LNKCAP_SLS_2_5GB)
> > +		return;
> > +
> > +	lnk_stat =3D cdns_pcie_rp_readw(pcie, pcie_cap_off +
> PCI_EXP_LNKSTA);
> > +	if ((lnk_stat & PCI_EXP_LNKSTA_CLS) =3D=3D PCI_EXP_LNKSTA_CLS_2_5GB)
> {
> > +		lnk_ctl =3D cdns_pcie_rp_readw(pcie,
> > +					     pcie_cap_off + PCI_EXP_LNKCTL);
> > +		lnk_ctl |=3D PCI_EXP_LNKCTL_RL;
> > +		cdns_pcie_rp_writew(pcie, pcie_cap_off + PCI_EXP_LNKCTL,
> > +				    lnk_ctl);
> > +
> > +		if (cdns_pcie_host_wait_for_link(pcie))
> > +			return;
> > +	}
> > +}
> >
> >  static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)  {
> > @@ -398,23 +442,6 @@ static int cdns_pcie_host_init(struct device *dev,
> >  	return cdns_pcie_host_init_address_translation(rc);
> >  }
> >
> > -static int cdns_pcie_host_wait_for_link(struct cdns_pcie *pcie) -{
> > -	struct device *dev =3D pcie->dev;
> > -	int retries;
> > -
> > -	/* Check if the link is up or not */
> > -	for (retries =3D 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;
> > -}
>=20
> This piece looks like a pure move of cdns_pcie_host_wait_for_link() with =
no
> other changes.  It would be nicer to split that to a separate patch so it=
 doesn't
> obscure what's really happening in this patch.
Yes, the api is moved to remove compilation error.
Will split the patch into two as suggested.

>=20
> >  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)  {
> >  	struct device *dev =3D rc->pcie.dev;
> > @@ -458,8 +485,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
> >  	}
> >
> >  	ret =3D cdns_pcie_host_wait_for_link(pcie);
> > -	if (ret)
> > +	if (ret) {
> >  		dev_dbg(dev, "PCIe link never came up\n");
> > +	} else {
> > +		if (rc->quirk_retrain_flag)
> > +			cdns_pcie_retrain(pcie);
> > +	}
> >
> >  	for (bar =3D RP_BAR0; bar <=3D RP_NO_BAR; bar++)
> >  		rc->avail_ib_bar[bar] =3D true;
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
> > b/drivers/pci/controller/cadence/pcie-cadence.h
> > index 30eba6cafe2c..0f29128a5d0a 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence.h
> > +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> > @@ -119,7 +119,7 @@
> >   * Root Port Registers (PCI configuration space for the root port func=
tion)
> >   */
> >  #define CDNS_PCIE_RP_BASE	0x00200000
> > -
> > +#define CDNS_PCIE_RP_CAP_OFFSET 0xc0
> >
> >  /*
> >   * Address Translation Registers
> > @@ -291,6 +291,7 @@ struct cdns_pcie {
> >   * @device_id: PCI device ID
> >   * @avail_ib_bar: Satus of RP_BAR0, RP_BAR1 and	RP_NO_BAR if it's free
> or
> >   *                available
> > + * @quirk_retrain_flag: Retrain link as quirk for PCIe Gen2
> >   */
> >  struct cdns_pcie_rc {
> >  	struct cdns_pcie	pcie;
> > @@ -299,6 +300,7 @@ struct cdns_pcie_rc {
> >  	u32			vendor_id;
> >  	u32			device_id;
> >  	bool			avail_ib_bar[CDNS_PCIE_RP_MAX_IB];
> > +	bool			quirk_retrain_flag;
> >  };
> >
> >  /**
> > @@ -414,6 +416,13 @@ static inline void cdns_pcie_rp_writew(struct
> cdns_pcie *pcie,
> >  	cdns_pcie_write_sz(addr, 0x2, value);  }
> >
> > +static inline u16 cdns_pcie_rp_readw(struct cdns_pcie *pcie, u32 reg)
> > +{
> > +	void __iomem *addr =3D pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> > +
> > +	return cdns_pcie_read_sz(addr, 0x2); }
> > +
> >  /* Endpoint Function register access */  static inline void
> > cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
> >  					  u32 reg, u8 value)
> > --
> > 2.17.1
> >