| Message ID | 20201215070009.27937-1-kishon@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] PCI: cadence: Retrain Link to work around Gen2 training defect. | expand |
On Tue, Dec 15, 2020 at 1:00 AM Kishon Vijay Abraham I <kishon@ti.com> 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 speed 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] -> 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. > + */ > + > + lnk_cap_sls = cdns_pcie_readl(pcie, (CDNS_PCIE_RP_BASE + pcie_cap_off + > + PCI_EXP_LNKCAP)); > + if ((lnk_cap_sls & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB) > + return; > + > + lnk_stat = cdns_pcie_rp_readw(pcie, pcie_cap_off + PCI_EXP_LNKSTA); > + if ((lnk_stat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB) { > + lnk_ctl = cdns_pcie_rp_readw(pcie, > + pcie_cap_off + PCI_EXP_LNKCTL); > + lnk_ctl |= 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 = 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; > -} > - > int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) > { > struct device *dev = rc->pcie.dev; > @@ -458,8 +485,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) > } > > ret = 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 = RP_BAR0; bar <= RP_NO_BAR; bar++) > rc->avail_ib_bar[bar] = 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 function) > */ > #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 = 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 >
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 speed 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] -> 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)) { - 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? - Where did the LINK_WAIT_* values come from? Are they derived from something in the PCIe spec? - Is the LINK_WAIT timeout related to LINK_RETRAIN_TIMEOUT? - If the PCI core does a link retrain, e.g., in pcie_retrain_link(), does that work correctly on this device? [I see now that this patch only *moves* this code without changing it. But I'm still curious.] > + 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. > + */ > + > + lnk_cap_sls = cdns_pcie_readl(pcie, (CDNS_PCIE_RP_BASE + pcie_cap_off + > + PCI_EXP_LNKCAP)); > + if ((lnk_cap_sls & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB) > + return; > + > + lnk_stat = cdns_pcie_rp_readw(pcie, pcie_cap_off + PCI_EXP_LNKSTA); > + if ((lnk_stat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB) { > + lnk_ctl = cdns_pcie_rp_readw(pcie, > + pcie_cap_off + PCI_EXP_LNKCTL); > + lnk_ctl |= 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 = 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; > -} 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. > int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) > { > struct device *dev = rc->pcie.dev; > @@ -458,8 +485,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) > } > > ret = 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 = RP_BAR0; bar <= RP_NO_BAR; bar++) > rc->avail_ib_bar[bar] = 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 function) > */ > #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 = 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 >
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@ti.com> 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 speed 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] -> 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. Thank You, Kishon
On Wed, Dec 16, 2020 at 9:01 AM Kishon Vijay Abraham I <kishon@ti.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@ti.com> 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 speed 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] -> 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
Hi Rob, On 16/12/20 10:31 pm, Rob Herring wrote: > On Wed, Dec 16, 2020 at 9:01 AM Kishon Vijay Abraham I <kishon@ti.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@ti.com> 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 speed 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] -> 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 For all GEN1 cards there will be re-train (since the Cadence IP RC is GEN2 or more say). This is going to be true for older Cadence IPs and newer Cadence IPs (where Cadence has enabled HW re-training). The quirk will prevent SW re-training for newer Cadence IPs when a GEN1 card is connected. > then 'advertised speed' would be lowered in that case (to prevent auto > retraining, right?) and the condition would be false. I don't think the value in PCI_EXP_LNKCAP will change for unstable links. But yeah it'll fall back to GEN1 based on link training and if the link is unstable it'll again fall back to GEN1 on link RE-training. Thanks, Kishon
On Fri, Dec 18, 2020 at 8:42 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi Rob, > > On 16/12/20 10:31 pm, Rob Herring wrote: > > On Wed, Dec 16, 2020 at 9:01 AM Kishon Vijay Abraham I <kishon@ti.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@ti.com> 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 speed 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] -> 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 > > For all GEN1 cards there will be re-train (since the Cadence IP RC is > GEN2 or more say). This is going to be true for older Cadence IPs and > newer Cadence IPs (where Cadence has enabled HW re-training). > > The quirk will prevent SW re-training for newer Cadence IPs when a GEN1 > card is connected. Okay, got it. Reviewed-by: Rob Herring <robh@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. > > EXTERNAL MAIL > > > 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 speed > 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 = { > > > > 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)) { > > - 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. > > - 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. > > - Is the LINK_WAIT timeout related to LINK_RETRAIN_TIMEOUT? Yes > > - If the PCI core does a link retrain, e.g., in pcie_retrain_link(), > does that work correctly on this device? Yes > > [I see now that this patch only *moves* this code without changing it. > But I'm still curious.] > > > + 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. > > + */ > > + > > + lnk_cap_sls = cdns_pcie_readl(pcie, (CDNS_PCIE_RP_BASE + > pcie_cap_off + > > + PCI_EXP_LNKCAP)); > > + if ((lnk_cap_sls & PCI_EXP_LNKCAP_SLS) <= > PCI_EXP_LNKCAP_SLS_2_5GB) > > + return; > > + > > + lnk_stat = cdns_pcie_rp_readw(pcie, pcie_cap_off + > PCI_EXP_LNKSTA); > > + if ((lnk_stat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB) > { > > + lnk_ctl = cdns_pcie_rp_readw(pcie, > > + pcie_cap_off + PCI_EXP_LNKCTL); > > + lnk_ctl |= 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 = 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; > > -} > > 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. > > > int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) { > > struct device *dev = rc->pcie.dev; > > @@ -458,8 +485,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) > > } > > > > ret = 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 = RP_BAR0; bar <= RP_NO_BAR; bar++) > > rc->avail_ib_bar[bar] = 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 function) > > */ > > #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 = 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 > >
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. + */ + + lnk_cap_sls = cdns_pcie_readl(pcie, (CDNS_PCIE_RP_BASE + pcie_cap_off + + PCI_EXP_LNKCAP)); + if ((lnk_cap_sls & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB) + return; + + lnk_stat = cdns_pcie_rp_readw(pcie, pcie_cap_off + PCI_EXP_LNKSTA); + if ((lnk_stat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB) { + lnk_ctl = cdns_pcie_rp_readw(pcie, + pcie_cap_off + PCI_EXP_LNKCTL); + lnk_ctl |= 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 = 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; -} - int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) { struct device *dev = rc->pcie.dev; @@ -458,8 +485,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) } ret = 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 = RP_BAR0; bar <= RP_NO_BAR; bar++) rc->avail_ib_bar[bar] = 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 function) */ #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 = 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)