On Wed, Dec 16, 2020 at 9:01 AM Kishon Vijay Abraham I <kishon@xxxxxx> 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@xxxxxx> wrote: > >> > >> From: Nadeem Athani <nadeem@xxxxxxxxxxx> > >> > >> 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@xxxxxxxxxxx> > >> [kishon@xxxxxx: Enable the workaround for TI's J721E SoC] > >> Signed-off-by: Kishon Vijay Abraham I <kishon@xxxxxx> > >> --- > >> 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@xxxxxxxxxxx > >> > >> 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
