On Fri, Dec 18, 2020 at 8:42 AM Kishon Vijay Abraham I <kishon@xxxxxx> 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@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 > > 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@xxxxxxxxxx>
